-
Notifications
You must be signed in to change notification settings - Fork 40
Fix incorrect validation causing maxThreads to be treated as required #3128
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 resolves a critical regression where the 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 fixes an issue where the --max-threads argument was incorrectly treated as required. The solution, changing the argument's type from int to Integer in the CLI options to allow for null values and then applying a default, is sound and has been applied consistently across both ExportCommand and ImportCommand. The related core options classes have also been refactored to handle default values more cleanly using @Builder.Default, and the accompanying tests are a great addition. My review includes a few suggestions to enhance maintainability by addressing duplicated code in the command classes and improving the robustness of the new CLI tests, which currently duplicate some implementation logic.
...-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommand.java
Show resolved
Hide resolved
...-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataimport/ImportCommand.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
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 refactors the default value handling for the maximum number of threads used in import and export operations, setting it to the number of available processors instead of a hardcoded value.
- Changed
maxThreads/maxThreadCountfields to use@Builder.DefaultwithRuntime.getRuntime().availableProcessors() - Updated CLI command classes to use
Integerinstead ofintfor proper null handling - Added comprehensive test coverage for both programmatic builder usage and CLI argument parsing
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ImportOptions.java | Added @Builder.Default annotation to maxThreads field with dynamic default value |
| ExportOptions.java | Added @Builder.Default annotation to maxThreadCount field and repositioned field declaration |
| ExportManager.java | Removed redundant default value logic now handled by ExportOptions builder |
| ImportCommandOptions.java | Changed maxThreads to Integer type and removed hardcoded defaultValue |
| ImportCommand.java | Added null check for maxThreads with runtime default assignment |
| ExportCommandOptions.java | Changed maxThreads to Integer type for nullable handling |
| ExportCommand.java | Added null check for maxThreads with runtime default assignment |
| ImportOptionsTest.java | New test file validating builder default behavior with and without explicit values |
| JsonExportManagerTest.java | Added tests for ExportOptions builder default behavior |
| ImportCommandTest.java | Added tests for CLI parsing and default value application |
| ExportCommandTest.java | Added tests for CLI parsing and default value application |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...-loader/cli/src/main/java/com/scalar/db/dataloader/cli/command/dataimport/ImportCommand.java
Show resolved
Hide resolved
...-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/dataimport/ImportOptions.java
Show resolved
Hide resolved
data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataexport/ExportOptions.java
Show resolved
Hide resolved
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! 👍
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.
LGTM! Thank you!
.../cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandOptions.java
Outdated
Show resolved
Hide resolved
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!
But, I left one comment. PTAL!
.../cli/src/main/java/com/scalar/db/dataloader/cli/command/dataexport/ExportCommandOptions.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!
|
@ypeckstadt Could you please resolve the conflicts? |
# Conflicts: # data-loader/cli/src/test/java/com/scalar/db/dataloader/cli/command/dataimport/ImportCommandTest.java
b07e70a to
cfc0d0b
Compare
|
@brfrn169 I have rebased the PR and fixed the conflicts. |
Description
This PR fixes a problem with the
--max-threadsargument being required when it should not be. I have to double-check, but I think this is a regression issue that was introduced in version 3.16.1. This PR fixes the issue by ensuring it is not required and that a proper default value is set.To avoid this from happening again, this PR adds extra tests as well and makes sure this problem is covered in both the Core and the CLI core.
Related issues and/or PRs
NA
Changes made
inttype for the argument withIntegerso it can properly be null-checked16default value that was set on import max threads. It should be the same run time based default value that is commonly used.max threadsargumentChecklist
Additional notes (optional)
This PR exposes a problem with the other CLI code. It needs to update the usage for other integer-based arguments as well and switch to
Integerwhere required. This will be handled in a separate PR.Release notes
Fixed incorrect validation causing maxThreads to be treated as required