-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HDFS-15934: Make DirectoryScanner reconcile blocks batch size and int… #2833
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
|
@Hexiaoqiao @ayushtkn @liuml07 |
|
💔 -1 overall
This message was automatically generated. |
ayushtkn
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.
Thanx @qizhu-lucas for the improvement.
Dropped a couple of comments give a check
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.
Can we add support for time units?
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.
Fixed it in latest PR.
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.
Add a validation for these configs, if reconcileBlocksBatchSize and reconcileBlocksBatchInterval is less than one use default. and add a warn log message if these values are incorrect something like:
Invalid value configured for < config name>, should be greater than 0, Using default.
In the end add an Info log for the values being used.
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.
Fixed in latest PR.
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.
can you add some recheck the descriptions for both the configs, the first line in. both is same, and doesn't make sense to me, recheck once.
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.
Changed in latest PR.
|
@ayushtkn |
|
💔 -1 overall
This message was automatically generated. |
ayushtkn
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.
Some minor comments, Otherwise the code seems good
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 should use conf.getTimeDuration, If you need some reference can check HDFS-15107, so as how to add support for time units.
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.
You need to format this code properly
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.
No need for jira id in the description, and this doesn't enable/disable, it is just to specify the batch size.
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.
Some grammatical error, Please rectify
|
Thanks very much for @ayushtkn patient review. |
|
💔 -1 overall
This message was automatically generated. |
ayushtkn
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.
Thanx for the update, and sorry for coming up late.
Dropped a minor comment. +1 once addressed
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.
change it to <=0
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.
change it to <=0
…a configurable size.
|
Thanks @ayushtkn for patient review. |
|
💔 -1 overall
This message was automatically generated. |
|
Thanx @qizhu-lucas for the contribution |
…erval between batch configurable. Contributed by Qi Zhu. (apache#2833) Signed-off-by: Ayush Saxena <[email protected]>
…erval between batch configurable.
NOTICE
Please create an issue in ASF JIRA before opening a pull request,
and you need to set the title of the pull request which starts with
the corresponding JIRA issue number. (e.g. HADOOP-XXXXX. Fix a typo in YYY.)
For more details, please see https://cwiki.apache.org/confluence/display/HADOOP/How+To+Contribute