-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
YARN-11463. Node Labels root directory creation doesn't have a retry logic #5562
Conversation
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.
@ashutoshcipher thanks for the patch! I had a comment.
// mkdir of root dir path | ||
fs.mkdirs(fsWorkingPath); | ||
// mkdir of root dir path with retry logic | ||
int maxRetries = 3; |
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 please create a configuration entry for the retry count and the interval? Something like the
yarn.resourcemanager.zk-num-retries and yarn.resourcemanager.zk-retry-interval-ms parameters defined for ZK connections. That way the user could configure a longer period in case HDFS in safe mode for a few minutes when the RM tries to start.
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 @brumi1024 for your review. I hve made this change in last commit
💔 -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.
Thanks @ashutoshcipher for the update. Apart from two nits it looks good to me. Can you please check the test failure?
@@ -137,6 +137,16 @@ private static void addDeprecatedKeys() { | |||
// Resource types configs | |||
//////////////////////////// | |||
|
|||
public static final String NODE_STORE_ROOT_DIR_NUM_RETRIES = | |||
YARN_PREFIX + "nodestore-rootdir.num-retries"; |
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.
Nit: Since this is an RM config (capacity scheduler feature) I think we could use the yarn.resourcemanager prefix.
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.
Makes sense. I will do that
public static final String NODE_STORE_ROOT_DIR_NUM_RETRIES = | ||
YARN_PREFIX + "nodestore-rootdir.num-retries"; | ||
|
||
public static final int NODE_STORE_ROOT_DIR_NUM_DEFAULT_RETRIES = 3; |
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.
Nit: To be inline with the other retries I think 500/1000 would be a more appropriate default.
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 will change it to 500
Thanks @brumi1024 for review. I will look at it. |
Hi @brumi1024. Sorry for getting a bit late. I have addressed your comments. Please review. Thanks |
🎊 +1 overall
This message was automatically generated. |
Thanks @ashutoshcipher for the patch, merged to trunk. |
Thanks @brumi1024 for review and merge. |
Description of PR
Node Labels root directory creation doesn't have a retry logic
JIRA - YARN-11463
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?