-
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
HDDS-2064. OzoneManagerRatisServer#newOMRatisServer throws NPE when OM HA is configured incorrectly #1398
Conversation
/label ozone |
@@ -614,6 +615,12 @@ private void loadOMHAConfigs(Configuration conf) { | |||
" system with " + OZONE_OM_SERVICE_IDS_KEY + " and " + | |||
OZONE_OM_ADDRESS_KEY; | |||
throw new OzoneIllegalArgumentException(msg); | |||
} else if (!isOMAddressSet && found == 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.
We cannot add this condition here, because think of case
OZONE_OM_SERVICE_IDS_KEY = ns1,ns2
Because after one iteration of nameserviceID if we are not able to find any om node matching with nameservice, we should not throw illegal configuration. We should try out all name services and see if it is matching with any nameservice.
And also, we can eliminate iteration of for loop with OmUtils.emptyAsSingletonNull(omServiceIds)) and similar for nameServiceIds. As for HA this is a must required configuration which needs to be set.
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.
And also try out test cases with multiple name services set also. And for this test case, you can use ozone.om.node.id if needed.
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.
@bharatviswa504 Thanks for the review. You are right. Now I understand that it makes sense to walk through all configured service ids before declaring failure.
I should probably just put found == 0
check outside the serviceId
loop.
/retest |
💔 -1 overall
This message was automatically generated. |
Thank you very much to open this pull request. During the weekend the Ozone source code has been moved out from apache/hadoop repository to apache/hadoop-ozone repository. This git commits are rewritten, but the branch of this pull request is also transformed (state of Saturday morning), you can use the new, migrated branch to recreate this pull request. Your pull request is important for us: Can you please re-create your pull request in the new repository? 1. Create a new fork of https://github.com/apache/hadoop-ozone 2. Clone it and have both your fork and the apache repo as remotes:
3. Fetch your migrated branch and push it to your fork.
4. And create the new pull request on the new repository. https://github.com/apache/hadoop-ozone/compare/master...smengcl:HDDS-2064?expand=1 If you need more information, please check this wiki page or contact with me (my github user name + apache.org). Thank you, and sorry for the inconvenience. |
Due to the refactoring done in HDDS-2162. This fix has been included in that commit. I will repurpose the jira to add a unit test for the HA config. I'm closing this PR. Will open another one in hadoop-ozone repo. |
The WIP patch can prevent NPE in
TestOzoneManagerConfiguration
unit testtestWrongConfigurationNoOMNodes
andtestWrongConfigurationNoOMAddrs
.Before the second commit, it fails
testWrongConfiguration
andtestMultipleOMServiceIds
. - The reasons of the failures are that there is another logic checkingisOMAddressSet
right after my patch inOzoneManager
. And those unit tests are expecting a different exception.The second commit has addressed the unit test failures. Please review.