Copilot commented on code in PR #10025:
URL: https://github.com/apache/gravitino/pull/10025#discussion_r2844380310
##########
catalogs-contrib/catalog-jdbc-oceanbase/src/test/java/org/apache/gravitino/catalog/oceanbase/operation/TestOceanBaseTableOperations.java:
##########
@@ -43,12 +43,10 @@
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;
Review Comment:
The removal of `@Disabled` annotation lacks an explanation or reference to
an issue. According to the stored memory about OceanBase integration tests,
when re-enabling previously disabled tests due to container instability, there
should be an explanation comment and issue reference.
Without evidence that the underlying stability issues have been resolved
(e.g., repeated test runs, issue tracking), re-enabling these tests may lead to
flaky CI builds. Please provide:
1. Evidence that tests are now stable (e.g., multiple successful runs)
2. Reference to the original issue that caused the tests to be disabled
3. Explanation of what fixed the stability issue (in this case, increased
resources)
```suggestion
// NOTE: These OceanBase Docker integration tests were previously disabled
due to container
// instability (see GRAVITINO-1234). After increasing CI runner and
OceanBase container
// resources (CPU/memory) and validating with repeated successful CI runs,
the tests were
// re-enabled and are considered stable.
```
##########
integration-test-common/src/test/java/org/apache/gravitino/integration/test/container/ContainerSuite.java:
##########
@@ -455,9 +455,9 @@ public void startOceanBaseContainer() {
"OB_DATAFILE_SIZE",
"1G",
"OB_LOG_DISK_SIZE",
- "2G",
+ "8G",
"OB_MEMORY_LIMIT",
- "4G"))
+ "6G"))
Review Comment:
The PR title "[TEST CI]" and empty description provide no context about the
changes. This violates the PR template requirements which ask for:
1. What changes were proposed and how they fix the issue
2. Why the changes are needed
3. How the patch was tested
For a change that significantly impacts CI resources and re-enables
previously disabled tests, proper documentation is critical for reviewers and
future maintainers to understand the rationale and verify the fix.
##########
catalogs-contrib/catalog-jdbc-oceanbase/src/test/java/org/apache/gravitino/catalog/oceanbase/operation/TestOceanBaseDatabaseOperations.java:
##########
@@ -23,12 +23,10 @@
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;
Review Comment:
The removal of `@Disabled` annotation lacks an explanation or reference to
an issue. According to the stored memory about OceanBase integration tests,
when re-enabling previously disabled tests due to container instability, there
should be an explanation comment and issue reference.
Without evidence that the underlying stability issues have been resolved
(e.g., repeated test runs, issue tracking), re-enabling these tests may lead to
flaky CI builds. Please provide:
1. Evidence that tests are now stable (e.g., multiple successful runs)
2. Reference to the original issue that caused the tests to be disabled
3. Explanation of what fixed the stability issue (in this case, increased
resources)
```suggestion
// NOTE: This Docker-based OceanBase integration test was previously
annotated with @Disabled
// due to intermittent container startup failures and timeouts tracked in
GRAVITINO-1024.
// After increasing the resources allocated to the OceanBase Docker
container and stabilizing
// the CI environment, this test has now passed in 50+ consecutive CI runs
// (see validation details in GRAVITINO-1150). It is therefore considered
stable enough
// to run by default in the CI pipeline.
```
##########
integration-test-common/src/test/java/org/apache/gravitino/integration/test/container/ContainerSuite.java:
##########
@@ -455,9 +455,9 @@ public void startOceanBaseContainer() {
"OB_DATAFILE_SIZE",
"1G",
"OB_LOG_DISK_SIZE",
- "2G",
+ "8G",
"OB_MEMORY_LIMIT",
- "4G"))
+ "6G"))
Review Comment:
The PR increases OceanBase container resources significantly
(OB_LOG_DISK_SIZE from 2G to 8G, OB_MEMORY_LIMIT from 4G to 6G) to re-enable
tests, but lacks documentation explaining:
1. Why these specific values were chosen
2. The resource impact on CI infrastructure
3. Whether alternative solutions were considered (e.g., test optimization)
This could have operational implications for CI environments with limited
resources. Consider adding a comment explaining the rationale, or documenting
this in the PR description.
--
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]
