Copilot commented on code in PR #10005:
URL: https://github.com/apache/gravitino/pull/10005#discussion_r2807447260
##########
integration-test-common/src/test/java/org/apache/gravitino/integration/test/container/ContainerSuite.java:
##########
@@ -428,16 +446,20 @@ public void startOceanBaseContainer() {
.withEnvVars(
ImmutableMap.of(
"MODE",
- "mini",
+ "MINI",
"OB_SYS_PASSWORD",
OceanBaseContainer.PASSWORD,
"OB_TENANT_PASSWORD",
OceanBaseContainer.PASSWORD,
"OB_DATAFILE_SIZE",
- "2G",
+ "1G",
"OB_LOG_DISK_SIZE",
+ "2G",
+ "OB_MEMORY_LIMIT",
"4G"))
.withNetwork(network)
+ .withFilesToMount(
+ ImmutableMap.<String,
String>builder().put("/tmp/obdata", "/root/ob").build())
Review Comment:
A new volume mount is added that maps the host directory /tmp/obdata to the
container path /root/ob. This directory is created earlier (lines 434-437), but
there's no cleanup mechanism to remove this directory after tests complete.
This could lead to accumulation of test data in /tmp/obdata across multiple
test runs, potentially causing disk space issues over time. Consider adding
cleanup logic in the container teardown or using a temporary directory that
gets automatically cleaned up.
##########
catalogs/catalog-jdbc-oceanbase/src/test/java/org/apache/gravitino/catalog/oceanbase/operation/TestOceanBaseTableOperations.java:
##########
@@ -43,10 +43,12 @@
import org.apache.gravitino.utils.RandomNameUtils;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Tag;
import org.junit.jupiter.api.Test;
@Tag("gravitino-docker-test")
+@Disabled
Review Comment:
The @Disabled annotation disables all OceanBase table operation tests. While
this might be necessary as a temporary workaround if the OceanBase container is
unstable in CI, disabling entire test classes without a comment explaining why,
when it will be re-enabled, or a tracking issue reference is problematic. This
reduces test coverage and the disabled tests could be forgotten.
Add a comment with the @Disabled annotation explaining why these tests are
disabled and reference a tracking issue for re-enabling them. For example:
@Disabled("Temporarily disabled due to OceanBase container resource constraints
in CI - see issue #XXXX")
##########
integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/ITUtils.java:
##########
@@ -206,5 +209,41 @@ public static String getBundleJarDirectory(String
bundleName) {
System.getenv("GRAVITINO_ROOT_DIR"), "bundles", bundleName, "build",
"libs");
}
+ public static void cleanDisk() {
+
+ Object output =
+ CommandExecutor.executeCommandLocalHost(
+ "df -h", false, ProcessData.TypesOfData.STREAMS_MERGED, Map.of());
+ LOG.info("Before clean: Command df -h output:\n{}", output);
+ output =
+ CommandExecutor.executeCommandLocalHost(
+ "free -m", false, ProcessData.TypesOfData.STREAMS_MERGED,
Map.of());
+ LOG.info("Before clean: Command free -m output:\n{}", output);
+
+ // Execute docker system prune -af to free up space before starting the
OceanBase container
+ ProcessBuilder processBuilder = new ProcessBuilder("/bin/bash", "-c",
"docker system prune -f");
+ try {
+ Process process = processBuilder.start();
+ int exitCode = process.waitFor();
+ if (exitCode != 0) {
+ throw new RuntimeException("Failed to execute free memory exit code: "
+ exitCode);
+ }
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to execute free memory command", e);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new RuntimeException("Interrupted while waiting for
util_free_space.sh to finish", e);
+ }
Review Comment:
The cleanDisk method uses ProcessBuilder directly, inconsistently with the
rest of the codebase which uses CommandExecutor for executing shell commands.
The CommandExecutor class already handles process execution, error logging, and
output capture. Using ProcessBuilder directly here bypasses the centralized
command execution logic and makes error handling inconsistent. Additionally,
the process output/error streams are not being captured or logged, which means
if the docker prune command fails, there won't be any diagnostic output to
understand why.
Consider using CommandExecutor.executeCommandLocalHost with appropriate
error handling instead of ProcessBuilder to maintain consistency with the rest
of the codebase.
```suggestion
Object pruneOutput =
CommandExecutor.executeCommandLocalHost(
"docker system prune -f", false,
ProcessData.TypesOfData.STREAMS_MERGED, Map.of());
LOG.info("Command docker system prune -f output:\n{}", pruneOutput);
```
##########
integration-test-common/src/test/java/org/apache/gravitino/integration/test/container/ContainerSuite.java:
##########
@@ -486,6 +510,7 @@ public void startLocalStackContainer() {
}
public void startStarRocksContainer() {
+ ITUtils.cleanDisk();
Review Comment:
The cleanDisk method is called before starting every single container type
(Hive, Trino, Doris, MySQL, PostgreSQL, OceanBase, Kafka, LocalStack,
StarRocks). This means running "docker system prune -f" and multiple diagnostic
commands (df -h, free -m) potentially dozens of times during a test run, which
will significantly slow down integration tests.
Docker system prune is an expensive operation that should only be called
when actually needed (e.g., when disk space is critically low, or specifically
before starting resource-intensive containers like OceanBase). Consider:
1. Only calling cleanDisk before specific containers that are known to
require significant disk space (like OceanBase)
2. Implementing a check to only clean when disk usage exceeds a threshold
3. Caching the last cleanup time to avoid repeated cleanups within a short
time window
##########
integration-test-common/src/test/java/org/apache/gravitino/integration/test/container/ContainerSuite.java:
##########
@@ -428,16 +446,20 @@ public void startOceanBaseContainer() {
.withEnvVars(
ImmutableMap.of(
"MODE",
- "mini",
+ "MINI",
Review Comment:
The MODE environment variable value is changed from "mini" to "MINI".
Environment variables in OceanBase container are case-sensitive, and this
change could potentially break the container startup if the OceanBase container
expects "mini" in lowercase. While this might be intentional to fix
compatibility with the newer container image version, it should be verified
that "MINI" is the correct value for OceanBase 4.2.1-lts. If this change is
based on updated OceanBase documentation or testing, it would be helpful to add
a comment explaining why the value was changed.
```suggestion
"mini",
```
##########
integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/ITUtils.java:
##########
@@ -206,5 +209,41 @@ public static String getBundleJarDirectory(String
bundleName) {
System.getenv("GRAVITINO_ROOT_DIR"), "bundles", bundleName, "build",
"libs");
}
+ public static void cleanDisk() {
+
+ Object output =
+ CommandExecutor.executeCommandLocalHost(
+ "df -h", false, ProcessData.TypesOfData.STREAMS_MERGED, Map.of());
+ LOG.info("Before clean: Command df -h output:\n{}", output);
+ output =
+ CommandExecutor.executeCommandLocalHost(
+ "free -m", false, ProcessData.TypesOfData.STREAMS_MERGED,
Map.of());
+ LOG.info("Before clean: Command free -m output:\n{}", output);
+
+ // Execute docker system prune -af to free up space before starting the
OceanBase container
+ ProcessBuilder processBuilder = new ProcessBuilder("/bin/bash", "-c",
"docker system prune -f");
+ try {
+ Process process = processBuilder.start();
+ int exitCode = process.waitFor();
+ if (exitCode != 0) {
+ throw new RuntimeException("Failed to execute free memory exit code: "
+ exitCode);
+ }
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to execute free memory command", e);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new RuntimeException("Interrupted while waiting for
util_free_space.sh to finish", e);
Review Comment:
The error message "Failed to execute free memory" is misleading and
inaccurate. The cleanDisk method is executing "docker system prune -f" to clean
up Docker resources, not freeing memory. The same issue exists on line 235
where the error mentions "util_free_space.sh" which is not the script being
executed. These error messages will confuse users when troubleshooting failures.
The error messages should accurately describe what operation failed, for
example: "Failed to execute docker system prune command" or "Interrupted while
waiting for docker cleanup to finish".
```suggestion
throw new RuntimeException(
"Failed to execute 'docker system prune -f' command, exit code:
" + exitCode);
}
} catch (IOException e) {
throw new RuntimeException("Failed to start 'docker system prune -f'
command", e);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new RuntimeException(
"Interrupted while waiting for 'docker system prune -f' command to
finish", e);
```
##########
integration-test-common/src/test/java/org/apache/gravitino/integration/test/container/ContainerSuite.java:
##########
@@ -418,6 +429,13 @@ public void startPostgreSQLContainer(TestDatabaseName
testDatabaseName) {
}
public void startOceanBaseContainer() {
+ ITUtils.cleanDisk();
+ // create dir /tmp/obdata by CommandExecutor
+ Object o =
+ CommandExecutor.executeCommandLocalHost(
+ "mkdir -p /tmp/obdata", false,
ProcessData.TypesOfData.STREAMS_MERGED);
+ LOG.info("Command mkdir -p /tmp/obdata output:\n{}", o);
Review Comment:
The creation of /tmp/obdata directory uses CommandExecutor but doesn't check
the exit code or handle errors. If the directory creation fails (e.g., due to
permissions), the output is only logged, and the container startup will
continue, likely failing later when trying to mount the non-existent directory.
Consider using CommandExecutor with error checking enabled
(IGNORE_ERRORS.FALSE) or verifying the directory was created successfully
before proceeding with container startup.
##########
integration-test-common/src/test/java/org/apache/gravitino/integration/test/container/ContainerSuite.java:
##########
@@ -428,16 +446,20 @@ public void startOceanBaseContainer() {
.withEnvVars(
ImmutableMap.of(
"MODE",
- "mini",
+ "MINI",
"OB_SYS_PASSWORD",
OceanBaseContainer.PASSWORD,
"OB_TENANT_PASSWORD",
OceanBaseContainer.PASSWORD,
"OB_DATAFILE_SIZE",
- "2G",
+ "1G",
"OB_LOG_DISK_SIZE",
+ "2G",
+ "OB_MEMORY_LIMIT",
"4G"))
Review Comment:
The OB_DATAFILE_SIZE is reduced from "2G" to "1G" and new environment
variables OB_MEMORY_LIMIT="4G" are added. Additionally, OB_LOG_DISK_SIZE value
appears to have been moved but stays as "4G" on line 459. However, the original
code shows OB_LOG_DISK_SIZE was also "4G" before. These changes modify the
OceanBase container resource allocation significantly. While this might be
necessary to work within CI runner constraints, the OB_MEMORY_LIMIT=4G seems
high if we're reducing other sizes due to space constraints. Please verify
these values are correct and won't cause the container to fail due to
insufficient resources or memory issues.
```suggestion
"2G"))
```
##########
.github/workflows/build.yml:
##########
@@ -15,7 +15,7 @@ concurrency:
# A workflow run is made up of one or more jobs that can run sequentially or
in parallel
jobs:
changes:
- runs-on: ubuntu-latest
+ runs-on: ubuntu-22.04
Review Comment:
The "changes" job is pinned to ubuntu-22.04 while other jobs in the workflow
remain on ubuntu-latest. This creates an inconsistency where different jobs
might run on different Ubuntu versions, potentially causing subtle differences
in behavior. If the purpose is to avoid issues with the newer GitHub runner
image, all jobs should be pinned to ubuntu-22.04 for consistency, or this
change should only be temporary with a plan to update to ubuntu-latest once
compatibility is verified. Consider documenting why only this specific job is
pinned.
```suggestion
runs-on: ubuntu-latest
```
##########
integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/ITUtils.java:
##########
@@ -206,5 +209,41 @@ public static String getBundleJarDirectory(String
bundleName) {
System.getenv("GRAVITINO_ROOT_DIR"), "bundles", bundleName, "build",
"libs");
}
+ public static void cleanDisk() {
+
+ Object output =
+ CommandExecutor.executeCommandLocalHost(
+ "df -h", false, ProcessData.TypesOfData.STREAMS_MERGED, Map.of());
+ LOG.info("Before clean: Command df -h output:\n{}", output);
+ output =
+ CommandExecutor.executeCommandLocalHost(
+ "free -m", false, ProcessData.TypesOfData.STREAMS_MERGED,
Map.of());
+ LOG.info("Before clean: Command free -m output:\n{}", output);
+
+ // Execute docker system prune -af to free up space before starting the
OceanBase container
+ ProcessBuilder processBuilder = new ProcessBuilder("/bin/bash", "-c",
"docker system prune -f");
Review Comment:
The comment on line 223 says "Execute docker system prune -af" but the
actual command being executed is "docker system prune -f" (missing the -a
flag). The -a flag is important as it removes all unused images, not just
dangling ones, which is what the comment suggests. Either the comment should be
corrected to match the actual command, or the command should include the -a
flag if that was the original intent.
```suggestion
ProcessBuilder processBuilder = new ProcessBuilder("/bin/bash", "-c",
"docker system prune -af");
```
##########
catalogs/catalog-jdbc-oceanbase/src/test/java/org/apache/gravitino/catalog/oceanbase/operation/TestOceanBaseDatabaseOperations.java:
##########
@@ -23,10 +23,12 @@
import java.util.Map;
import org.apache.gravitino.utils.RandomNameUtils;
import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Tag;
import org.junit.jupiter.api.Test;
@Tag("gravitino-docker-test")
+@Disabled
Review Comment:
The @Disabled annotation disables all OceanBase database operation tests.
While this might be necessary as a temporary workaround if the OceanBase
container is unstable in CI, disabling entire test classes without a comment
explaining why, when it will be re-enabled, or a tracking issue reference is
problematic. This reduces test coverage and the disabled tests could be
forgotten.
Add a comment with the @Disabled annotation explaining why these tests are
disabled and reference a tracking issue for re-enabling them. For example:
@Disabled("Temporarily disabled due to OceanBase container resource constraints
in CI - see issue #XXXX")
--
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]
