Backport to branch(3.15) : Add JDBC database permission test (Part 2) #2941
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 aimed at improving the robustness and compatibility of JDBC database permission integration tests. It introduces specific DDL synchronization waits for YugabyteDB to ensure test stability and refines user management and permission granting logic for Db2, alongside general refactoring of DDL waiting mechanisms in the test suite.
Highlights
- Enhanced JDBC Permission Testing Robustness: Introduced a standardized
waitForDdlCompletion()mechanism across JDBC permission integration tests, specifically adding a sleep for YugabyteDB to prevent schema/catalog version mismatch errors during DDL operations. - Improved Db2 Compatibility for Test Users: Modified JDBC permission test utilities to correctly handle user creation, dropping, and permission granting for Db2, by skipping unsupported operations and providing Db2-specific grant statements.
- Test Infrastructure Refactoring: Renamed
waitForTableCreation()towaitForDdlCompletion()in base permission test classes and their implementations for better clarity and consistency, and added DDL waits in test setup routines. - Minor Test Schema Adjustment: Updated the test table metadata in
DistributedStorageAdminPermissionIntegrationTestBaseto change the data type ofCOL_NAME3fromTEXTtoINT.
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 database permission tests, including specific handling for Yugabyte and DB2. The changes introduce necessary waits for DDL operations and fix a data type inconsistency. However, there is a critical issue where the code for DB2 support seems incomplete, referencing an undefined method isDb2, which will cause compilation to fail. I've also suggested a minor improvement to avoid re-initializing a class member on every method call for better efficiency and maintainability.
| 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(rdbEngine) is used here and in other places in this file, but it doesn't seem to be defined in JdbcTestUtils based on the provided file context. Similarly, RdbEngineFactory doesn't appear to support DB2.
This will cause a compilation error. It seems that some related changes for DB2 support might be missing from this backport. Please verify that all necessary dependencies and definitions for DB2 are included.
| .addColumn(COL_NAME1, DataType.INT) | ||
| .addColumn(COL_NAME2, DataType.TEXT) | ||
| .addColumn(COL_NAME3, DataType.TEXT) | ||
| .addColumn(COL_NAME3, DataType.INT) |
There was a problem hiding this comment.
The column COL_NAME3 is being redefined from TEXT to INT. Ensure that this change is intentional and does not break compatibility or existing data.
| 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.
The rdbEngine field is re-initialized every time getProperties() is called. This method can be invoked multiple times during the test setup, leading to unnecessary object creation. Consider initializing rdbEngine only once using a null check.
Properties properties = JdbcEnv.getProperties(testName);
if (rdbEngine == null) {
rdbEngine = RdbEngineFactory.create(new JdbcConfig(new DatabaseConfig(properties)));
}
return properties;| 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.
The rdbEngine field is re-initialized every time getProperties() is called. This method can be invoked multiple times during the test setup, leading to unnecessary object creation. Consider initializing rdbEngine only once using a null check.
Properties properties = JdbcEnv.getProperties(testName);
if (rdbEngine == null) {
rdbEngine = RdbEngineFactory.create(new JdbcConfig(new DatabaseConfig(properties)));
}
return properties;
2 participants
This is an automated backport of the following:
Please merge this PR after all checks have passed.