-
Notifications
You must be signed in to change notification settings - Fork 40
Fix to not drop the metadata table when renaming table #3071
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly addresses a potential NOT FOUND
exception when renaming a table if the metadata table does not exist. The fix, which involves ensuring the metadata table is recreated, is sound. The addition of unit and integration tests is also great, especially the integration test that specifically covers the scenario where only one table exists. I have a couple of suggestions to improve the maintainability of the test code by reducing duplication.
core/src/test/java/com/scalar/db/storage/jdbc/JdbcAdminTest.java
Outdated
Show resolved
Hide resolved
...ion-test/src/main/java/com/scalar/db/api/DistributedTransactionAdminIntegrationTestBase.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug in the rename table feature where the metadata table was not being recreated if it didn't exist, causing potential failures when inserting new metadata after renaming.
Key Changes:
- Modified
JdbcAdmin.renameTable()
to recreate the metadata table if it doesn't exist by changing theaddTableMetadata
parameter fromfalse
totrue
- Added integration tests to verify table renaming works correctly when only one table exists in a namespace
- Updated unit tests to verify metadata table recreation SQL statements are executed during rename operations
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
File | Description |
---|---|
core/src/main/java/com/scalar/db/storage/jdbc/JdbcAdmin.java | Changed parameter in addTableMetadata call to enable metadata table recreation |
core/src/test/java/com/scalar/db/storage/jdbc/JdbcAdminTest.java | Added expected SQL statements for metadata table creation across all database types |
integration-test/src/main/java/com/scalar/db/api/DistributedTransactionAdminIntegrationTestBase.java | Added integration test for renaming when only one table exists |
integration-test/src/main/java/com/scalar/db/api/DistributedStorageAdminIntegrationTestBase.java | Added integration test for renaming when only one table exists |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
@komamitsu @Torch3333 I revised this PR to avoid dropping the metadata table instead of recreating it. Please take a look when you get a chance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you!
Description
This PR updates the rename table feature to avoid dropping the metadata table if it is empty.
Currently, the feature drops the metadata table if it is empty. Therefore, it can fail due to the metadata table NOT FOUND exception when inserting new metadata if only one table exists:
Related issues and/or PRs
Changes made
renameTable
.Checklist
Additional notes (optional)
N/A
Release notes
N/A