HBASE-29365 Improve option parser of PerformanceEvaluation #7052
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
junegunn
commented
May 31, 2025
| .load(PerformanceEvaluation.class.getClassLoader().getResourceAsStream(fileName)); | ||
| opts.commandProperties = properties; | ||
| } catch (IOException e) { | ||
| LOG.error("Failed to load metricIds from properties file", e); |
There was a problem hiding this comment.
I changed two things here.
- The original error message was a bit strange. Where did
metricIdscome from? I changed it toFailed to load command properties from file - It used to print the error message and proceed. I decided to throw the exception instead.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors option parsing in PerformanceEvaluation to use dynamic mappings and implements stricter boolean parsing. Key changes include replacing the long if‐chain for options, adding new test cases for the parser, and introducing a custom parseBoolean method for robust flag handling.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| hbase-diagnostics/src/test/java/org/apache/hadoop/hbase/TestPerformanceEvaluation.java | Added tests to verify parsing behavior, including missing values and boolean flag handling. |
| hbase-diagnostics/src/main/java/org/apache/hadoop/hbase/PerformanceEvaluation.java | Refactored option parsing with maps for flag and value handling and improved error reporting. |
hbase-diagnostics/src/test/java/org/apache/hadoop/hbase/TestPerformanceEvaluation.java
Outdated
Show resolved
Hide resolved
Apache9
reviewed
Jun 1, 2025
| continue; | ||
| } | ||
| if (cmd.startsWith("--")) { | ||
| final String[] parts = cmd.substring(2).split("=", 2); |
There was a problem hiding this comment.
Better use Splitter from our shaded guava?
There was a problem hiding this comment.
Thanks for the suggestion. I like that we can use trimResults with Splitter.
Addressed in 28b2769.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the option parser in PerformanceEvaluation to reduce duplicated code and improve boolean parsing strictness while adding tests for the new behavior.
- Refactor of option parsing to use maps for both boolean flags and value-based options
- Addition of a custom parseBoolean method for stricter boolean handling
- New tests in TestPerformanceEvaluation to validate parsing scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| hbase-diagnostics/src/test/java/org/apache/hadoop/hbase/TestPerformanceEvaluation.java | Added tests to validate boolean flags and missing option values |
| hbase-diagnostics/src/main/java/org/apache/hadoop/hbase/PerformanceEvaluation.java | Refactored option parsing via maps and introduced strict boolean parsing with parseBoolean |
Comments suppressed due to low confidence (1)
hbase-diagnostics/src/main/java/org/apache/hadoop/hbase/PerformanceEvaluation.java:2967
- [nitpick] Clarify the error message in parseBoolean to specify that only 'true' or 'false' values are accepted.
private static boolean parseBoolean(String value) {
hbase-diagnostics/src/main/java/org/apache/hadoop/hbase/PerformanceEvaluation.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
approved these changes
Jun 2, 2025
|
Please raise PR for branch-2.x? Thanks. |
|
Thanks for the review! Just opened a pull request to branch-2: #7056 |
|
Also to branch-2.6: #7057 |
Apache9
pushed a commit
that referenced
this pull request
Jun 3, 2025
Signed-off-by: Duo Zhang <[email protected]> (cherry picked from commit 97cf65d)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://issues.apache.org/jira/browse/HBASE-29365