Backport to branch(3.14) : Add JDBC database permission test (Part 2) #2942
Conversation
Co-authored-by: Mitsunori Komatsu <[email protected]> Co-authored-by: Toshihiro Suzuki <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @feeblefakie, 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 is an automated backport to branch 3.14, focusing on improving the robustness and compatibility of JDBC database permission integration tests. It refactors the DDL waiting logic to be more generic and introduces database-specific adjustments for YugabyteDB and DB2 to ensure test stability and proper permission handling across different relational database systems.
Highlights
- Refactoring DDL Wait Mechanism: The pull request generalizes the DDL (Data Definition Language) waiting mechanism across various integration tests. The method
waitForTableCreation()has been renamed and refactored towaitForDdlCompletion()in base classes and specific Cassandra/JDBC permission tests. This provides a more generic and consistent way to ensure DDL operations have completed before proceeding with tests. - JDBC Permission Test Enhancements for Database Compatibility: Specific handling has been introduced for different JDBC database types within the permission tests. For YugabyteDB, a short sleep duration (
DDL_WAIT_SECONDS) is now applied after DDL operations to mitigate potential schema or catalog version mismatch issues. Additionally, user creation/deletion operations are conditionally skipped for DB2, and DB2-specificGRANTstatements have been added to ensure proper permission testing for that database. - Test Table Schema Adjustment: A minor adjustment was made to the test table schema defined in
DistributedStorageAdminPermissionIntegrationTestBase.java, changing the data type ofCOL_NAME3fromTEXTtoINT. This likely aligns the test schema with updated requirements or more realistic test scenarios.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request is a backport that adds JDBC permission tests, focusing on YugabyteDB and DB2. The changes refactor how tests wait for DDL operations to complete, adding specific delays for YugabyteDB to improve test stability.
My review identified a critical compilation error due to a missing isDb2 utility method, which suggests an incomplete backport. I've also pointed out an opportunity to improve maintainability by refactoring duplicated code in the new JDBC test classes into a common base class.
| String createUserSql = getCreateUserSql(userName, password); | ||
| try (Statement statement = connection.createStatement()) { | ||
| statement.execute(createUserSql); | ||
| if (!JdbcTestUtils.isDb2(rdbEngine)) { |
There was a problem hiding this comment.
The method JdbcTestUtils.isDb2() is used on this line, and also on lines 38 and 136, but it is not defined in the JdbcTestUtils class. This will cause a compilation error.
It seems this backport is incomplete. Please add the isDb2 method and any other related DB2 changes to resolve this.
| private RdbEngineStrategy rdbEngine; | ||
|
|
||
| @Override | ||
| protected Properties getProperties(String testName) { | ||
| return JdbcEnv.getProperties(testName); | ||
| Properties properties = JdbcEnv.getProperties(testName); | ||
| rdbEngine = RdbEngineFactory.create(new JdbcConfig(new DatabaseConfig(properties))); | ||
| return properties; | ||
| } |
There was a problem hiding this comment.
There is significant code duplication between this class (JdbcAdminPermissionIntegrationTest) and JdbcPermissionIntegrationTest. Both classes introduce an rdbEngine field, initialize it within getProperties, and implement identical logic to wait for DDL operations on YugabyteDB.
To improve maintainability and reduce redundancy, consider creating a common abstract base class for JDBC permission tests that would contain this shared logic.
For example, an abstract base class could manage the rdbEngine and provide a shared waitForDdlCompletionForYugabyte() method. The concrete test classes would then extend this base class and use the shared functionality.
2 participants
This is an automated backport of the following:
Please merge this PR after all checks have passed.