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
HBASE-25781 Add cacheBlocks option to RowCounter #3177
base: master
Are you sure you want to change the base?
Conversation
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
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.
A few suggested changes otherwise looks good.
@@ -64,13 +64,15 @@ | |||
|
|||
private final static String OPT_START_TIME = "starttime"; | |||
private final static String OPT_END_TIME = "endtime"; | |||
private final static String OPT_IS_CACHE_BLOCKS = "isCacheBlocks"; |
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 be named 'cacheBlocks'?
private final static String OPT_RANGE = "range"; | ||
private final static String OPT_EXPECTED_COUNT = "expectedCount"; | ||
|
||
private String tableName; | ||
private List<MultiRowRangeFilter.RowRange> rowRangeList; | ||
private long startTime; | ||
private long endTime; | ||
private boolean isCacheBlocks; |
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.
isCacheBlocks is name of the method that looks at the cacheBlock data member content, not the name of the data member... so suggest s/isCacheBlocks/cacheBlocks/
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.
Okay, I'll fix it right away, as well as the changes I missed to submit last time
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
this.startTime = getOptionAsLong(cmd, OPT_START_TIME, 0L); | ||
this.endTime = getOptionAsLong(cmd, OPT_END_TIME, HConstants.LATEST_TIMESTAMP); | ||
this.expectedCount = getOptionAsLong(cmd, OPT_EXPECTED_COUNT, Long.MIN_VALUE); | ||
this.cacheBlocks = cmd.hasOption(OPT_CACHE_BLOCKS) ? true : false; |
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.cacheBlocks = cmd.hasOption(OPT_CACHE_BLOCKS);
Is enough no?
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.
What u mean?
The state var is a variable and assign it with o/p of the check is enough.
We dont need like if hasOption() the set to true or else set to false.
Its a minor thing though.
So instead of having
this.cacheBlocks = cmd.hasOption(OPT_CACHE_BLOCKS) ? true : false;
Below is enough
this.cacheBlocks = cmd.hasOption(OPT_CACHE_BLOCKS) ;
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.
I made a ridiculous mistake. I'll correct it right away. Thank you very much
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
No description provided.