-
Notifications
You must be signed in to change notification settings - Fork 40
Refactor TableMetadataService to Use DistributedTransactionAdmin Instead of DistributedStorageAdmin #3146
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
Refactor TableMetadataService to Use DistributedTransactionAdmin Instead of DistributedStorageAdmin #3146
Conversation
Summary of ChangesHello @inv-jishnu, 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 a crucial step towards ensuring ScalarDB Cluster compatibility for the Data Loader components. By migrating Highlights
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
|
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 refactors TableMetadataService to use DistributedTransactionAdmin instead of DistributedStorageAdmin, which is an important step for ScalarDB Cluster compatibility. The changes are applied consistently across the affected classes, including the unit tests.
I have one main point of feedback regarding a potential resource leak in ExportCommand.java. The newly introduced DistributedTransactionAdmin is an AutoCloseable resource but is not being closed, which could lead to issues. I've left a comment with a suggestion on how to fix this by using a try-with-resources statement, similar to how it's handled in ImportCommand.java.
Overall, this is a good and necessary refactoring. Addressing the resource management issue will make it perfect.
...-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java
Outdated
Show resolved
Hide resolved
ypeckstadt
left a comment
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.
...-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java
Outdated
Show resolved
Hide resolved
|
@thongdk8 |
thongdk8
left a comment
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!
brfrn169
left a comment
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.
Left a comment. Other than that, LGTM! Thank you!
...-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java
Show resolved
Hide resolved
data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataexport/ExportManager.java
Show resolved
Hide resolved
Torch3333
left a comment
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!
feeblefakie
left a comment
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
left a comment
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!
…ead of DistributedStorageAdmin (#3146) Co-authored-by: Hiroyuki Yamada <mogwaing@gmail.com>
Description
This pull request updates the TableMetadataService class to replace the usage of DistributedStorageAdmin with DistributedTransactionAdmin.
This change is part of the ongoing effort to ensure ScalarDB Cluster compatibility, as the DistributedStorageAdmin interface is not supported in cluster mode. By switching to DistributedTransactionAdmin, the service now uses the transactional administration API, enabling schema and metadata operations to function correctly in both standalone and clustered ScalarDB environments.
This refactor aligns with the broader migration of the Data Loader components from storage-based to transaction-based APIs. Other areas that still depend on DistributedStorage will be addressed in future PRs.
Related issues and/or PRs
Please review and merge this PR once the following PR is merged
Changes made
Checklist
Additional notes (optional)
@inv-jishnu : We need to change the target branch to master after the related PRs are merged.
Release notes
Refactor TableMetadataService to Use DistributedTransactionAdmin Instead of DistributedStorageAdmin