-
Notifications
You must be signed in to change notification settings - Fork 40
Add deprecated Data Loader CLI arguments #3041
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
base: master
Are you sure you want to change the base?
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 focuses on enhancing the usability and stability of the data loader by reintroducing specific command-line arguments that were previously removed. The primary objective is to ensure that existing scripts and workflows relying on these arguments continue to function seamlessly, thereby preventing disruptions for users. Additionally, the changes include robust validation mechanisms to guide users toward the correct argument usage and prevent conflicts between old and new options. 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.
Pull Request Overview
This PR restores backward compatibility for the ScalarDB data loader CLI by re-adding three deprecated CLI arguments (--threads
, --start-exclusive
, and --end-exclusive
) that were renamed during code migration. The deprecated options are hidden from help but remain functional, with validation to prevent users from specifying both old and new options simultaneously.
- Adds deprecated CLI options with hidden visibility to maintain backward compatibility
- Implements validation logic to prevent conflicting deprecated and new option usage
- Adds comprehensive test coverage for the validation scenarios
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
DataLoaderError.java | Adds new error code for deprecated/new option conflict validation |
ImportCommandOptions.java | Adds deprecated --threads option and logic to apply deprecated values |
ImportCommand.java | Implements validation and application of deprecated thread option |
ExportCommandOptions.java | Adds deprecated --start-exclusive and --end-exclusive options with application logic |
ExportCommand.java | Implements validation for deprecated start/end exclusive options |
ImportCommandTest.java | Adds test coverage for deprecated thread option validation |
ExportCommandTest.java | Adds test coverage for deprecated start/end exclusive option validation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...der/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandTest.java
Show resolved
Hide resolved
...der/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandTest.java
Show resolved
Hide resolved
...der/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataimport/ImportCommandTest.java
Show resolved
Hide resolved
.../cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandOptions.java
Show resolved
Hide resolved
...der/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandTest.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.
Code Review
This pull request re-adds deprecated CLI arguments to maintain backward compatibility, which is a good improvement. The implementation is solid, including validation to prevent conflicts between old and new arguments, and tests to cover these new validations. I've provided a couple of suggestions to refactor duplicated code in both the main logic and the tests, which will improve the maintainability of the code.
...-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java
Show resolved
Hide resolved
...der/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandTest.java
Show resolved
Hide resolved
data-loader/core/src/main/java/com/scalar/db/dataloader/core/DataLoaderError.java
Show resolved
Hide resolved
data-loader/core/src/main/java/com/scalar/db/dataloader/core/DataLoaderError.java
Outdated
Show resolved
Hide resolved
…ataLoaderError.java Co-authored-by: Mitsunori Komatsu <komamitsu@gmail.com>
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!
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 Is it ok for me to update the Data Loader JAR files on the release page for 3.16.0 and 3.16.1 once this PR is merged into the 3.16 branch? Or it is better to handle this in a different way? |
@ypeckstadt We can leave the existing JARs as they are and release the bug fix in the next patch version, |
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! But, I left a question. PTAL!
@CommandLine.Option( | ||
names = {DEPRECATED_THREADS_OPTION}, | ||
paramLabel = "<THREADS>", | ||
description = "Deprecated: Use --max-threads instead", |
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.
Just a question, but why threads
are renamed to max-threads
?
It creates threads dynamically within the max, so that the number could be lower than the max depending on workloads?
Description
This PR re-adds 3 CLI arguments that have been renamed during the data loader code migration from the standalone repository to the main ScalarDB repository and are causing backwards compatibility issues.
Those 3 CLI arguments being:
--threads
--start-exclusive
--end-exclusive
This PR makes sure the backwards compatibility is restored by re-adding them back as deprecated CLI arguments that do not show up in
--help
but are still there and working.Related issues and/or PRs
Issue showing up after reviewing the data-loader documentation
Changes made
Checklist
Additional notes (optional)
This would need to be updated in ScalarDB 3.16.0 and 3.16.1 release to really fix it. I suggest this one time I can just overwrite the jar file in the release for each version if allowed instead of doing a patch version.
Release notes
NA