Skip to content
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

Closed
wants to merge 7 commits into from

Conversation

smengcl
Copy link
Contributor

@smengcl smengcl commented Sep 3, 2019

The WIP patch can prevent NPE in TestOzoneManagerConfiguration unit test testWrongConfigurationNoOMNodes and testWrongConfigurationNoOMAddrs.

Before the second commit, it fails testWrongConfiguration and testMultipleOMServiceIds. - The reasons of the failures are that there is another logic checking isOMAddressSet right after my patch in OzoneManager. And those unit tests are expecting a different exception.

The second commit has addressed the unit test failures. Please review.

@smengcl smengcl changed the title [WIP] HDDS-2064. OzoneManagerRatisServer#newOMRatisServer throws NPE when OM HA is configured incorrectly HDDS-2064. OzoneManagerRatisServer#newOMRatisServer throws NPE when OM HA is configured incorrectly Sep 3, 2019
@smengcl
Copy link
Contributor Author

smengcl commented Sep 3, 2019

/label ozone

@elek elek added the ozone label Sep 3, 2019
@@ -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) {
Copy link
Contributor

@bharatviswa504 bharatviswa504 Sep 3, 2019

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@smengcl
Copy link
Contributor Author

smengcl commented Sep 4, 2019

/retest

@hadoop-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
0 reexec 4861 Docker mode activated.
_ Prechecks _
+1 dupname 0 No case conflicting files found.
+1 @author 0 The patch does not contain any @author tags.
+1 test4tests 0 The patch appears to include 1 new or modified test files.
_ trunk Compile Tests _
0 mvndep 33 Maven dependency ordering for branch
+1 mvninstall 620 trunk passed
+1 compile 408 trunk passed
+1 checkstyle 89 trunk passed
+1 mvnsite 0 trunk passed
+1 shadedclient 891 branch has no errors when building and testing our client artifacts.
+1 javadoc 194 trunk passed
0 spotbugs 515 Used deprecated FindBugs config; considering switching to SpotBugs.
+1 findbugs 751 trunk passed
_ Patch Compile Tests _
0 mvndep 31 Maven dependency ordering for patch
+1 mvninstall 615 the patch passed
+1 compile 424 the patch passed
+1 javac 424 the patch passed
+1 checkstyle 92 the patch passed
+1 mvnsite 0 the patch passed
+1 whitespace 0 The patch has no whitespace issues.
+1 shadedclient 681 patch has no errors when building and testing our client artifacts.
+1 javadoc 189 the patch passed
+1 findbugs 692 the patch passed
_ Other Tests _
+1 unit 300 hadoop-hdds in the patch passed.
-1 unit 199 hadoop-ozone in the patch failed.
+1 asflicense 48 The patch does not generate ASF License warnings.
11309
Reason Tests
Failed junit tests hadoop.ozone.om.ratis.TestOzoneManagerDoubleBufferWithOMResponse
Subsystem Report/Notes
Docker Client=19.03.1 Server=19.03.1 base: https://builds.apache.org/job/hadoop-multibranch/job/PR-1398/1/artifact/out/Dockerfile
GITHUB PR #1398
Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname Linux e3eaa0d1930b 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/hadoop.sh
git revision trunk / dc9abd2
Default Java 1.8.0_222
unit https://builds.apache.org/job/hadoop-multibranch/job/PR-1398/1/artifact/out/patch-unit-hadoop-ozone.txt
Test Results https://builds.apache.org/job/hadoop-multibranch/job/PR-1398/1/testReport/
Max. process+thread count 1328 (vs. ulimit of 5500)
modules C: hadoop-ozone/ozone-manager hadoop-ozone/integration-test U: hadoop-ozone
Console output https://builds.apache.org/job/hadoop-multibranch/job/PR-1398/1/console
versions git=2.7.4 maven=3.3.9 findbugs=3.1.0-RC1
Powered by Apache Yetus 0.10.0 http://yetus.apache.org

This message was automatically generated.

@elek
Copy link
Member

elek commented Oct 13, 2019

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:

git clone git@github.com:smengcl/hadoop-ozone.git
cd hadoop-ozone
git remote add apache git@github.com:apache/hadoop-ozone.git
git fetch apache

3. Fetch your migrated branch and push it to your fork.

git checkout -b HDDS-2064 apache/HDDS-2064
git push origin HDDS-2064

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.

@smengcl
Copy link
Contributor Author

smengcl commented Nov 5, 2019

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.

@smengcl smengcl closed this Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants