-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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-24137:The max merge count of metafixer may be remind in hbase-site.xml #1478
Conversation
🎊 +1 overall
This message was automatically generated. |
@@ -63,7 +61,7 @@ | |||
MetaFixer(MasterServices masterServices) { | |||
this.masterServices = masterServices; | |||
this.maxMergeCount = this.masterServices.getConfiguration(). | |||
getInt(MAX_MERGE_COUNT_KEY, MAX_MERGE_COUNT_DEFAULT); | |||
getInt("hbase.master.metafixer.max.merge.count", 10); |
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.
It was configurable before this change? You could add
hbase.master.metafixer.max.merge.count 11 configure the max merge count...
to your hbase-site.xml and away you go?
But maybe you think it deserves mention in the hbase-default.xml so operators can find it easier? If so, I'd suggest a fuller description.
Thanks.
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 think should be add hbase.master.metafixer.max.merge.count in hbase-default.xml that so operators can change it easier. I have added fuller description in hbase-default.xml.
Thanks
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.
Yes if the purpose is to mention this config in hbase-default.xml
, good to also document it in hbase-default.adoc
with detailed description.
However, in that case, title of PR doesn't match. As @saintstack said, this is already configurable.
Thanks.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Thank @gentlewangyu for opening the PR, it makes reviews much easier than the attached patch. As for the effects of this change, I've left similar comments over on the Jira. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@ndimiduk @saintstack @virajjasani thanks for your suggestion, I see . |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Thanks @gentlewangyu , this looks good. Can you also add this to hbase-default.adoc if not available already? |
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.
Thanks @gentlewangyu , this looks good. Can you also add this to hbase-default.adoc if not available already?
I have added this to hbase-default.adoc .
Thanks
@@ -63,7 +61,7 @@ | |||
MetaFixer(MasterServices masterServices) { | |||
this.masterServices = masterServices; | |||
this.maxMergeCount = this.masterServices.getConfiguration(). | |||
getInt(MAX_MERGE_COUNT_KEY, MAX_MERGE_COUNT_DEFAULT); | |||
getInt("hbase.master.metafixer.max.merge.count", 10); |
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 think should be add hbase.master.metafixer.max.merge.count in hbase-default.xml that so operators can change it easier. I have added fuller description in hbase-default.xml.
Thanks
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
this description that I reference the calculateMerges method of MetaFixer Thanks |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Be aware that HBASE-24246 changes the default value from 10..... |
💔 -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.
Looks good except has wrong default. Thanks.
@@ -1946,6 +1946,14 @@ possible configurations would overwhelm and obscure the important. | |||
responses with complete data. | |||
</description> | |||
</property> | |||
<property> | |||
<name>hbase.master.metafixer.max.merge.count</name> | |||
<value>10</value> |
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.
The default changed.
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.
@saintstack I have changed the default value
Thanks
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
No description provided.