Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16321 )
Change subject: [master] KUDU-2181 Raft change config request for adding a master ...................................................................... Patch Set 4: (18 comments) I looked through quickly. Overall looks good, I think I'll do a second pass tomorrow. http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/common/wire_protocol.proto File src/kudu/common/wire_protocol.proto: http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/common/wire_protocol.proto@127 PS4, Line 127: be drop http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/catalog_manager.h@784 PS4, Line 784: std::pair<consensus::RaftPeerPB::Role, consensus::RaftPeerPB::MemberType> nit: does it make sense to add a typedef for this? http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/catalog_manager.h@812 PS4, Line 812: bool add Consider introducing an enum for this: it improves the readability of the code at call sites and brings more consistency to the signature of the method. http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/catalog_manager.h@1119 PS4, Line 1119: const Drop 'const' because this is not a truly constant method (you can reset the underlying pointer given the returned shared_ptr). See https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/const.md for details. http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/catalog_manager.h@1119 PS4, Line 1119: consensus Is it possible to reuse CatalogManager::master_consensus() instead of introducing this new method? http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc File src/kudu/master/dynamic_multi_master-test.cc: http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@154 PS4, Line 154: "--master_addresses=" + JoinStrings(master_addresses, ","), : "--rpc_reuseport=true", : "--master_support_change_config", : "--master_address_add_new_master=" + reserved_hp_str_, : "--logtostderr", : "--logbuflevel=-1" Does it make sense to retrieve the 'common' part of options for master from opts_.extra_master_flags ? http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@202 PS4, Line 202: CHECK_OK Would RETURN_NOT_OK() suffice here? http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@240 PS4, Line 240: StartCluster nit: wrap into NO_FATALS() or make StartCluster() return status and wrap into ASSERT_OK() to bail from the test scenario earlier if an error occurs within StartCluster()? http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@243 PS4, Line 243: master drop http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@245 PS4, Line 245: VerifyNumMastersAndGetAddresses nit: wrap into NO_FATALS() http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@253 PS4, Line 253: AddMasterToCluster Does it make sense to verify that an attempt of initiating master config change via a non-leader master returns an error, as expected? http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@261 PS4, Line 261: rpc::RpcController nit here and elsewhere: it's possible to add 'using kudu::rpc::RpcController;' and the omit 'rpc' namespace prefix throughout the code in this file http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@262 PS4, Line 262: leader_master nit: maybe, leader_master_idx would be more appropriate name for this variable? http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@270 PS4, Line 270: ASSERT_TRUE(master.role() == consensus::RaftPeerPB::LEADER || : master.role() == consensus::RaftPeerPB::FOLLOWER); : } Does it make sense to add a sanity check to make sure there is only one leader in the list? http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@287 PS4, Line 287: Adding the same master Does is make sense to verify and an attempt to add any of the former masters returns an error as well? http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@295 PS4, Line 295: ASSERT_OK(cluster_->GetLeaderMasterIndex(&leader_master)); : ASSERT_TRUE( : cluster_->master_proxy(leader_master)->AddMaster(req, &resp, &rpc).IsRemoteError()); Here and elsewhere, the leader master might change in between: this might be a source of flakiness in the future (especially in TSAN builds). Probably, it makes sense to wrap this into some sort of ASSERT_EVENTUALLY() or alike. http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@432 PS4, Line 432: TestAddMasterWithNoLastKnownAddr In addition, does it make sense to add a test to verify that that the system behaves as expected when '--master_support_change_config' is omitted or set to 'false'? http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/master.proto@913 PS4, Line 913: required I think this should be 'optional' as in other requests because we are planning to upgrade to proto3 eventually. -- 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: 4 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: Wed, 16 Sep 2020 02:52:30 +0000 Gerrit-HasComments: Yes
