-
Notifications
You must be signed in to change notification settings - Fork 40
Deprecate --mode option in Import Command and auto-determine mode from transaction manager configuration #3177
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
Summary of ChangesHello @ypeckstadt, 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 streamlines the data import process by eliminating the need for a redundant 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 effectively deprecates the --mode CLI option, simplifying the user experience by automatically determining the import mode from the transaction manager configuration. The implementation is robust, providing clear deprecation warnings while maintaining backward compatibility. The internal refactoring, including the introduction of the TransactionMode enum with more descriptive names, significantly enhances code clarity and maintainability. The accompanying tests are thorough and correctly validate the new automatic mode detection logic. I have one suggestion to further improve the code's robustness by preventing a potential NullPointerException. Overall, this is a high-quality contribution.
data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataimport/ImportManager.java
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,16 @@ | |||
| package com.scalar.db.dataloader.cli; | |||
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.
Should not be merged until #3147 is completed. Have to update the target branch before that.
| * CLI option. | ||
| */ | ||
| @Deprecated | ||
| public enum ScalarDbMode { |
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.
While it may seem strange to create a new class that is deprecated right away, this class is added to the cli code as the original class, and its enums have been renamed in the data loader core code for clarity.
| * The available transaction modes for data import operations. Determines how data is imported based | ||
| * on the transaction manager configuration. | ||
| */ | ||
| public enum TransactionMode { |
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.
This class replaces the ScalarDbMode enum class.
| // refactored later | ||
| // and removed so that the whole code can just use one way to import all the data with the | ||
| // correct interface and not depend on making this distinction. | ||
| TransactionMode transactionMode = |
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.
This check is mainly required because in the data loader for code, we have to make a difference between transactional and non-transactional. It might be worth checking to see if we can do a refactor in the future to avoid this kind of check, though. But for now, it's required; otherwise, too much refactoring needs to be done.
|
Can be ignored and will be resolved in other PRs. |
Description
The
--modeCLI option in the Import Command is being deprecated because the import mode should be determined by the transaction manager configuration in the ScalarDB properties file, not by a separate CLI argument. This makes the data loader behavior consistent with the ScalarDB configuration and simplifies the command-line interface.Previously, users had to specify
--mode STORAGEor--mode TRANSACTIONin addition to configuring their ScalarDB transaction manager. This was redundant and could lead to confusion if the CLI mode didn't match the actual transaction manager configuration.With this change, the import mode is automatically determined from the transaction manager type:
SingleCrudOperationTransactionManageris configured → uses single-crud modeRelated issues and/or PRs
This PR currently targets the feat/data-loader/import-replace-storage to make it easier to remove. It has to target
masterbranch before merging.Should we merged after the following is completed
Refactor Data Loader Import to Use DistributedTransactionManager for Storage Mode #3147
Changes made
CLI Changes
--mode(-m) option inImportCommand--modeoption is usedScalarDbModeenum in CLI module with legacySTORAGEandTRANSACTIONvalues for backward compatibilityCore Changes
scalarDbModeparameter fromImportManagerconstructorImportManager.startImport()based on transaction manager type (instanceof check)ScalarDbModetoTransactionModefor claritySTORAGE→SINGLE_CRUD(clarifies single-crud operation mode)TRANSACTION→CONSENSUS_COMMIT(clarifies consensus commit protocol)ImportProcessorand related classes to use newTransactionModeenumTest Changes
startImport_shouldUseStorageMode_whenTransactionManagerIsSingleCrud()→startImport_shouldUseSingleCrudMode_whenTransactionManagerIsSingleCrud()SINGLE_CRUD,CONSENSUS_COMMIT)getScalarDbMode()instead ofgetTransactionMode()Checklist
Additional notes (optional)
The deprecated
--modeoption will still be accepted (with a warning) to maintain backward compatibility for existing scripts and automation.The option is hidden from help output. Users should remove this option from their scripts and rely on the transaction manager configuration in their ScalarDB properties file instead.
The enum renaming (
ScalarDbMode→TransactionMode,STORAGE→SINGLE_CRUD,TRANSACTION→CONSENSUS_COMMIT) is an internal change that provides clearer semantics and better aligns with ScalarDB terminology. The CLI module maintains a separate deprecatedScalarDbModeenum for backward compatibility.Release notes
Deprecated --mode option in Import Command and auto-determine mode from transaction manager configuration