Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16321 )
Change subject: [master] KUDU-2181 Raft ChangeConfig request to add a master ...................................................................... Patch Set 5: (4 comments) Some more nits. http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/catalog_manager.h@810 PS5, Line 810: kRemoveMaster > This will be tested in a follow-up patch, right? +1 It would be great to explicitly mention this in the commit description (if the idea is to add corresponding tests in a follow-up patch indeed). http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc File src/kudu/master/dynamic_multi_master-test.cc: http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@186 PS5, Line 186: ASSERT_EVENTUALLY([&] { > nit: why do we need this? I guess the idea was to protect against fluctuations of master leadership (at least I mentioned that in one of the comments in PS4, and I mentioned ASSERT_EVENTUALLY might help there). Maybe, instead of using ASSERT_EVENTUALLY on any failure, these should be handling MasterErrorPB::NOT_THE_LEADER failures only? Another approach could be using some sort of a function that finds the leader master and calls the specified functor, retrying the operation if MasterErrorPB::NOT_THE_LEADER is returned back? http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@518 PS5, Line 518: a master with missing master address Not sure whether I missed it or not, but what about the case of trying to add a master which is unreachable (e.g., currently shutdown or outright non-routable IP address)? http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@524 PS5, Line 524: master nit: drop -- To view, visit http://gerrit.cloudera.org:8080/16321 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0ac7e6e55220bcb01cad0fa386daaf656258088c Gerrit-Change-Number: 16321 Gerrit-PatchSet: 5 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-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 21 Sep 2020 22:58:33 +0000 Gerrit-HasComments: Yes
