WIP: Add integration test configuration for YubagyteDB version 2025#3338
WIP: Add integration test configuration for YubagyteDB version 2025#3338komamitsu wants to merge 4 commits into
Conversation
Summary of ChangesHello @komamitsu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on expanding the integration test suite to include YugabyteDB version 2025.x. It involves updating the CI configuration to support this new version, making necessary adjustments to specific integration tests to ensure compatibility and stability with YugabyteDB's characteristics, and adding utility methods for environment detection. The overall goal is to enhance the robustness of the testing infrastructure for different database backends. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces integration test configuration for YugabyteDB, specifically targeting version 2025. The changes include parameterizing the YugabyteDB test setup in ci/tests-config.yaml to support multiple versions, adding a utility method isYugabyte() in JdbcEnv.java for better database identification, and adjusting the thread count for YugabyteDB in JdbcDatabaseCrossPartitionScanIntegrationTest.java and JdbcDatabaseMultipleClusteringKeyScanIntegrationTest.java. A critical fix was also applied in DistributedStorageCrossPartitionScanIntegrationTestBase.java to ensure the rdbEngine is properly initialized before getThreadNum() is called, which is essential for correct test execution with YugabyteDB.
There was a problem hiding this comment.
Code Review
This pull request adds integration test configuration for YugabyteDB version 2025. It correctly parameterizes the CI configuration to run tests against multiple YugabyteDB versions and includes necessary workarounds in the test code, such as limiting execution to a single thread for YugabyteDB tests. The change also includes an important fix for the test setup initialization order, preventing a potential NullPointerException. The changes are logical and well-implemented. I've added a couple of suggestions to reduce code duplication for better maintainability.
| @Override | ||
| protected int getThreadNum() { | ||
| if (JdbcTestUtils.isOracle(rdbEngine)) { | ||
| if (JdbcTestUtils.isOracle(rdbEngine) || JdbcTestUtils.isYugabyte(rdbEngine)) { |
There was a problem hiding this comment.
The condition JdbcTestUtils.isOracle(rdbEngine) || JdbcTestUtils.isYugabyte(rdbEngine) is also used in JdbcDatabaseMultipleClusteringKeyScanIntegrationTest. To avoid code duplication and improve maintainability, consider adding a new helper method in JdbcTestUtils, for example isSingleThreadedForTest(), and use it in both places.
Example in JdbcTestUtils:
public static boolean isSingleThreadedForTest(RdbEngineStrategy rdbEngine) {
return isOracle(rdbEngine) || isYugabyte(rdbEngine);
}Then you can simplify this to:
if (JdbcTestUtils.isSingleThreadedForTest(rdbEngine)) {| @Override | ||
| protected int getThreadNum() { | ||
| if (JdbcTestUtils.isOracle(rdbEngine)) { | ||
| if (JdbcTestUtils.isOracle(rdbEngine) || JdbcTestUtils.isYugabyte(rdbEngine)) { |
There was a problem hiding this comment.
This condition is duplicated in JdbcDatabaseCrossPartitionScanIntegrationTest. To improve maintainability, consider creating a shared helper method in JdbcTestUtils to encapsulate this logic. For example:
// in JdbcTestUtils.java
public static boolean isSingleThreadedForTest(RdbEngineStrategy rdbEngine) {
return isOracle(rdbEngine) || isYugabyte(rdbEngine);
}This would allow you to simplify the condition here and in the other test class.
There was a problem hiding this comment.
Pull request overview
Updates CI/integration-test configuration to run JDBC integration and permission tests against YugabyteDB 2025.x, and adjusts integration test behavior to accommodate Yugabyte-specific concurrency limitations.
Changes:
- Add YugabyteDB 2025.x as an additional JDBC integration-test CI target (alongside existing 2.x).
- Add a dedicated GitHub Actions permission integration-test job for YugabyteDB 2025.x.
- Reduce test parallelism (thread count and parallel DDL) for YugabyteDB, and reorder executor initialization to occur after properties/engine setup.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| integration-test/src/main/java/com/scalar/db/api/DistributedStorageCrossPartitionScanIntegrationTestBase.java | Initialize executor after properties-based factory creation (supports subclasses that determine thread count after reading properties). |
| core/src/integration-test/java/com/scalar/db/storage/jdbc/JdbcEnv.java | Add YugabyteDB JDBC URL detection helper. |
| core/src/integration-test/java/com/scalar/db/storage/jdbc/JdbcDatabaseMultipleClusteringKeyScanIntegrationTest.java | Force single-thread execution for YugabyteDB runs. |
| core/src/integration-test/java/com/scalar/db/storage/jdbc/JdbcDatabaseCrossPartitionScanIntegrationTest.java | Force single-thread execution for YugabyteDB runs. |
| ci/tests-config.yaml | Convert YugabyteDB CI entry into a versioned matrix including 2025.1.0.1-b3. |
| .github/workflows/permission-check.yaml | Add a new YugabyteDB 2025 permission integration-test job. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - label: yugabytedb_%VERSION% | ||
| display_name: YugabyteDB %VERSION% | ||
| versions: | ||
| # YugabyteDB manages a global schema catalog version. A session using a stale version fails. | ||
| # Our integration tests address this limitation with some workarounds. | ||
| # However, version 2024.x is more sensitive to the schema catalog version mismatch, so we skip it. | ||
| # Since the YugabyteDB team aims to remove this limitation in 2025.x, the integration tests pass with that version. | ||
| - 2.20.4.0-b50 | ||
| - 2025.1.0.1-b3 | ||
| setup: "docker run -p 5433:5433 -e YSQL_USER=yugabyte -e YSQL_PASSWORD=yugabyte -d yugabytedb/yugabyte:%VERSION% bin/yugabyted start --background=false --master_flag=\"ysql_enable_db_catalog_version_mode=false\" --tserver_flags=\"ysql_enable_db_catalog_version_mode=false\"" | ||
| run: ./gradlew integrationTestJdbc -Dscalardb.jdbc.url=jdbc:yugabytedb://localhost:5433/?load-balance=any -Dscalardb.jdbc.username=yugabyte -Dscalardb.jdbc.password=yugabyte -Dscalar.db.jdbc.connection_pool.max_total=12 -Dscalar.db.jdbc.table_metadata.connection_pool.max_total=4 -Dscalar.db.jdbc.admin.connection_pool.max_total=4 |
There was a problem hiding this comment.
The PR description is still the default template/WIP and doesn't explain the motivation, scope, or how to validate these CI changes. Please update the PR description (especially the 'Description', 'Changes made', and testing/validation notes) so reviewers can assess the impact of adding YugabyteDB 2025 coverage.
Description
Related issues and/or PRs
Changes made
Checklist
Additional notes (optional)
Release notes