Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16936 )
Change subject: [master] KUDU-2181 Raft ChangeConfig request to remove a master ...................................................................... Patch Set 3: Code-Review+2 (5 comments) Overall looks good to me, just a few nits. http://gerrit.cloudera.org:8080/#/c/16936/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16936/1//COMMIT_MSG@12 PS1, Line 12: tests > That's a good point and I guess applies to both addition and removal of mas Sure! SGTM. http://gerrit.cloudera.org:8080/#/c/16936/3/src/kudu/master/dynamic_multi_master-test.cc File src/kudu/master/dynamic_multi_master-test.cc: http://gerrit.cloudera.org:8080/#/c/16936/3/src/kudu/master/dynamic_multi_master-test.cc@253 PS3, Line 253: returns nit: return? Or change 'Verify' --> 'Verifies' http://gerrit.cloudera.org:8080/#/c/16936/3/src/kudu/master/dynamic_multi_master-test.cc@950 PS3, Line 950: ASSERT_TRUE(s.IsRemoteError()); nit here and elsewhere in similar ASSERT: just in case if it ever fails/triggers, consider outputting s.ToString() in such cases -- it helps with debugging; i.e. something like ASSERT_TRUE(s.IsRemoteError()) << s.ToString(); Or make this assert to be EXPECT_TRUE(), so the following ASSERT_STR_MATCHES() would output more information on the error. I guess the former choice if preferable because it's more universal. http://gerrit.cloudera.org:8080/#/c/16936/3/src/kudu/master/master.cc File src/kudu/master/master.cc: http://gerrit.cloudera.org:8080/#/c/16936/3/src/kudu/master/master.cc@533 PS3, Line 533: nit: stick the ampersand to the type (i.e. to the 'auto') http://gerrit.cloudera.org:8080/#/c/16936/3/src/kudu/master/master.cc@575 PS3, Line 575: return Status::IllegalState(Substitute("Found multiple masters with same RPC address $0 " : "and UUID $1", hp.ToString(), uuid)); I guess this might be changed into LOG(FATAL)? First of, this should not be possible, and if it happened, how to get out of such a situation? What do you think? -- To view, visit http://gerrit.cloudera.org:8080/16936 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I76c03b8850faef60b65f85184c0a4db7cc6759ee Gerrit-Change-Number: 16936 Gerrit-PatchSet: 3 Gerrit-Owner: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 01 Feb 2021 22:55:18 +0000 Gerrit-HasComments: Yes
