-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HDDS-999. Make the DNS resolution in OzoneManager more resilient. (swagle) #758
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
|
💔 -1 overall
This message was automatically generated. |
bharatviswa504
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.
I have a few comments, rest LGTM.
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.
We removed WAITFOR env usage, there are few other files where this is used, om-statefulset.yaml. Do we need to remove from there also?
And also we are removing usage of WAITFOR, then do we need to remove the logic for this in docker image code?
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.
Making the changes to remove the wait for, @elek it makes sense to remove the wait for from everywhere, right?
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, it should be fine AFAIK.
|
💔 -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.
ENSURE_OM_INITIALIZED is not required here. For kubernetes we follow a different (more generic) approach. We can define an 'initContainer' to execute the 'om --init' first. (as you can see this is the initContainer here). As the 'om --init' can be executed multiple times it works well without the environment variable (which is handled by the starter script of the container.
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 for the review @elek. Removed in the latest commit.
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, it should be fine AFAIK.
|
💔 -1 overall
This message was automatically generated. |
elek
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.
+1. Thanks the update. Will merge it soon...
Author: [email protected] <[email protected]> Reviewers: Prateek Maheshwari <[email protected]> Closes apache#758 from rmatharu/doc
Brought back change from HDDS-776 with the retriable task, OM will now wait for at least 50 seconds before giving up.