Bankim Bhavsar 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: (17 comments) Thanks for the review, Alexey! 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 Done 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? Done 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 c Done 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 intro master_consensus() is used by catalog manager when it may not be fully initialized. So can't use that function. I agree having an additional consensus function could cause confusion. My goal was to put common code between Role() and GetRoleAndMemberType() in a common function but it's just a couple of lines. So removing it instead as simply checking for existing IsInitialized() function looks sufficient. 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 See below. 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 Done 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? Done 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 in Done http://gerrit.cloudera.org:8080/#/c/16321/4/src/kudu/master/dynamic_multi_master-test.cc@243 PS4, Line 243: master > drop Done 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 ch Done 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::RpcControlle Done 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 varia Done 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 lea Done 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 master Done 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 b Done 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 syste Done 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 plann Done -- 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: Thu, 17 Sep 2020 17:25:48 +0000 Gerrit-HasComments: Yes
