Copilot commented on code in PR #64113:
URL: https://github.com/apache/doris/pull/64113#discussion_r3354956498
##########
regression-test/suites/external_table_p0/paimon/test_paimon_jdbc_catalog.groovy:
##########
@@ -66,38 +66,58 @@ suite("test_paimon_jdbc_catalog", "p0,external") {
assertTrue(jdbcDriversDir != null && !jdbcDriversDir.isEmpty(),
"jdbc_drivers_dir must be configured")
- def executeCommand = { String cmd, Boolean mustSuc ->
+ def executeCommand = { String cmd, Boolean mustSuc, int timeoutSeconds =
300 ->
+ StringBuilder stdout = new StringBuilder()
+ StringBuilder stderr = new StringBuilder()
try {
logger.info("execute ${cmd}")
- def proc = new ProcessBuilder("/bin/bash", "-c",
cmd).redirectErrorStream(true).start()
- int exitcode = proc.waitFor()
- String output = proc.text
+ def proc = new ProcessBuilder("/bin/bash", "-c", cmd).start()
+ proc.consumeProcessOutput(stdout, stderr)
+ proc.waitForOrKill(timeoutSeconds * 1000)
Review Comment:
`timeoutSeconds * 1000` is computed as an `int`, which can overflow if
larger values are ever passed (and it’s easy to accidentally do so when reusing
this helper). Use a `long` calculation (e.g., multiply by `1000L`) to ensure
correct millisecond conversion.
##########
regression-test/suites/external_table_p0/paimon/test_paimon_jdbc_catalog.groovy:
##########
@@ -66,38 +66,58 @@ suite("test_paimon_jdbc_catalog", "p0,external") {
assertTrue(jdbcDriversDir != null && !jdbcDriversDir.isEmpty(),
"jdbc_drivers_dir must be configured")
- def executeCommand = { String cmd, Boolean mustSuc ->
+ def executeCommand = { String cmd, Boolean mustSuc, int timeoutSeconds =
300 ->
+ StringBuilder stdout = new StringBuilder()
+ StringBuilder stderr = new StringBuilder()
try {
logger.info("execute ${cmd}")
- def proc = new ProcessBuilder("/bin/bash", "-c",
cmd).redirectErrorStream(true).start()
- int exitcode = proc.waitFor()
- String output = proc.text
+ def proc = new ProcessBuilder("/bin/bash", "-c", cmd).start()
+ proc.consumeProcessOutput(stdout, stderr)
+ proc.waitForOrKill(timeoutSeconds * 1000)
+ int exitcode = proc.exitValue()
Review Comment:
`waitForOrKill` does not guarantee the process has fully terminated at the
moment `exitValue()` is called; `exitValue()` can throw
`IllegalThreadStateException` if the process is still alive. Consider
explicitly checking `proc.isAlive()` after `waitForOrKill(...)` (and treating
it as a timeout failure), or switch to a wait-with-timeout approach that
returns completion status before calling `exitValue()`.
##########
regression-test/suites/external_table_p0/paimon/test_paimon_jdbc_catalog.groovy:
##########
@@ -66,38 +66,58 @@ suite("test_paimon_jdbc_catalog", "p0,external") {
assertTrue(jdbcDriversDir != null && !jdbcDriversDir.isEmpty(),
"jdbc_drivers_dir must be configured")
- def executeCommand = { String cmd, Boolean mustSuc ->
+ def executeCommand = { String cmd, Boolean mustSuc, int timeoutSeconds =
300 ->
+ StringBuilder stdout = new StringBuilder()
+ StringBuilder stderr = new StringBuilder()
try {
logger.info("execute ${cmd}")
- def proc = new ProcessBuilder("/bin/bash", "-c",
cmd).redirectErrorStream(true).start()
- int exitcode = proc.waitFor()
- String output = proc.text
+ def proc = new ProcessBuilder("/bin/bash", "-c", cmd).start()
+ proc.consumeProcessOutput(stdout, stderr)
+ proc.waitForOrKill(timeoutSeconds * 1000)
+ int exitcode = proc.exitValue()
+ String output = stdout.toString()
+ String error = stderr.toString()
if (exitcode != 0) {
- logger.info("exit code: ${exitcode}, output\n: ${output}")
+ logger.info("exit code: ${exitcode}, stdout\n:
${output}\nstderr\n: ${error}")
if (mustSuc) {
- assertTrue(false, "Execute failed: ${cmd}")
+ assertTrue(false, "Execute failed:
${cmd}\nstdout:\n${output}\nstderr:\n${error}")
}
}
return output
} catch (IOException e) {
- assertTrue(false, "Execute timeout: ${cmd}")
+ assertTrue(false, "Execute failed: ${cmd}, err: ${e.message}")
}
}
- executeCommand("mkdir -p ${localDriverDir}", false)
- executeCommand("mkdir -p ${jdbcDriversDir}", true)
+ executeCommand("mkdir -p ${localDriverDir}", false, 60)
+ executeCommand("mkdir -p ${jdbcDriversDir}", true, 60)
if (!new File(localDriverPath).exists()) {
- executeCommand("/usr/bin/curl --max-time 600 ${driverDownloadUrl}
--output ${localDriverPath}", true)
+ executeCommand("/usr/bin/curl --max-time 600 ${driverDownloadUrl}
--output ${localDriverPath}", true, 660)
}
- executeCommand("cp -f ${localDriverPath} ${jdbcDriversDir}/${driverName}",
true)
+ executeCommand("cp -f ${localDriverPath} ${jdbcDriversDir}/${driverName}",
true, 60)
- String sparkContainerName = executeCommand("docker ps --filter
name=spark-iceberg --format {{.Names}}", false)
+ String sparkContainerName = executeCommand("docker ps --filter
name=spark-iceberg --format {{.Names}}", false, 30)
?.trim()
if (sparkContainerName == null || sparkContainerName.isEmpty()) {
logger.info("spark-iceberg container not found, skip this test")
return
}
- executeCommand("docker cp ${localDriverPath}
${sparkContainerName}:${sparkDriverPath}", true)
+ executeCommand("docker cp ${localDriverPath}
${sparkContainerName}:${sparkDriverPath}", true, 60)
+
+ String sparkMinioEndpoint = "http://${externalEnvIp}:${minioPort}";
+ if (sparkContainerName.contains("spark-iceberg")) {
+ String sparkMinioContainerName =
sparkContainerName.replaceFirst("spark-iceberg", "minio")
+ String resolvedSparkMinioContainer = executeCommand(
+ "docker ps --filter name=${sparkMinioContainerName} --format
{{.Names}}",
+ false,
+ 30
+ )?.trim()
+ if (resolvedSparkMinioContainer == sparkMinioContainerName) {
+ // Spark runs inside the docker network and may not be able to
reach the host-mapped MinIO port.
+ sparkMinioEndpoint = "http://${resolvedSparkMinioContainer}:9000";
+ }
Review Comment:
`docker ps --format {{.Names}}` can return multiple matching container names
separated by newlines when the filter matches more than one container. In that
case, `resolvedSparkMinioContainer == sparkMinioContainerName` will never be
true, and the test will keep using the host-mapped endpoint even though an
in-network MinIO container exists. Consider parsing the output (e.g., first
non-empty line) and checking for non-empty results rather than strict equality.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
