-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
HDFS-16400. Reconfig DataXceiver parameters for datanode #3843
Conversation
🎊 +1 overall
This message was automatically generated. |
Hi @tasanuma @Hexiaoqiao @ayushtkn @ferhui , could you please take a look at this. Thanks. |
String result; | ||
try { | ||
LOG.info("Reconfiguring {} to {}", property, newVal); | ||
if (property.equals(DFS_DATANODE_MAX_RECEIVER_THREADS_KEY)) { |
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 if-statement seems redundant since it is already checked by case DFS_DATANODE_MAX_RECEIVER_THREADS_KEY
.
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 @tasanuma for your review. I agree with your suggestion. There is only one parameter at present and it has been judged in advance. I will fix it.
@@ -68,8 +68,7 @@ | |||
* Enforcing the limit is required in order to avoid data-node | |||
* running out of memory. | |||
*/ | |||
int maxXceiverCount = | |||
DFSConfigKeys.DFS_DATANODE_MAX_RECEIVER_THREADS_DEFAULT; | |||
volatile int maxXceiverCount; |
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 like we need volatile for sure right? There might be slightest chance for DataXceiverServer thread to throw limit exceeded error if this is not volatile?
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 @virajjasani for your review. IMO, to ensure consistency across threads, we need to make it volatile.
...p-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDataNodeReconfiguration.java
Outdated
Show resolved
Hide resolved
💔 -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.
LGTM.
Hi @virajjasani , could you please review this again. Thank you. |
One checkstyle warning is pending, else looks good. |
Thanks @virajjasani for your comments. This parameter If we change |
Yeah since it is not newly introduced and we are updating it to volatile, it's fine to leave it as is rather than making it private. I believe we can ignore this checkstyle warning in this case. |
Thanks for the review. I'm merging it. |
Thanks for your contribution, @tomscut. Thanks for your review, @virajjasani. |
Thanks @tasanuma and @virajjasani for your review and merge. |
Reviewed-by: Viraj Jasani <[email protected]> Signed-off-by: Takanobu Asanuma <[email protected]> (cherry picked from commit f02374d)
Cherry-picked it into branch-3.3. |
Reviewed-by: Viraj Jasani <[email protected]> Signed-off-by: Takanobu Asanuma <[email protected]> (cherry picked from commit f02374d)
Reviewed-by: Viraj Jasani <[email protected]> Signed-off-by: Takanobu Asanuma <[email protected]>
JIRA: HDFS-16400.
To avoid frequent rolling restarts of the DN, we should make DataXceiver parameters reconfigurable.