Bankim Bhavsar 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: (16 comments) 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 > +1 Remove master is not implemented. Removing the enum. http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/catalog_manager.cc@1419 PS5, Line 1419: > nit: 4 spaces here Done 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@126 PS5, Line 126: resp > Should we also check resp.error() here? Same elsewhere. Done http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@149 PS5, Line 149: // Start with an existing master daemon. > nit: maybe, "Start with an existing master daemon's options, but modify the Done http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@186 PS5, Line 186: ASSERT_EVENTUALLY([&] { > I guess the idea was to protect against fluctuations of master leadership ( Yeah it was to prevent flakiness in tests due to leadership change between querying for leader master and issuing leader master RPC. Added a functor approach instead. http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@194 PS5, Line 194: num_masters_ > nit: this number changes throughout the test, so it isn't obvious what its Changed to orig_num_masters_ and check using orig_num_masters_ as the base. http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@198 PS5, Line 198: returning the status. > nit: could you mention how this might fail, so it's obvious when and why we Done http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@258 PS5, Line 258: ASSERT_EVENTUALLY > nit: in general, it isn't obvious why we need ASSERT_EVENTUALLYs everywhere Done http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@273 PS5, Line 273: num_masters_ > nit: I'd much rather have this be constant and explicitly use `orig_num_mas Done http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@399 PS5, Line 399: SleepFor(MonoDelta::FromSeconds(1)); > nit: could we instead look at metrics to wait until WAL GC has happened? I took a look around but couldn't determine the right metric that would suggest sys catalog WAL has been GC'ed or what it's current size is to determine that event has happened. Added TODO and keeping the sleep here for now. http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@515 PS5, Line 515: }); > nit: could we also run VerifyNumMastersAndGetAddresses here too? Same with Done 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 a Done http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/dynamic_multi_master-test.cc@524 PS5, Line 524: master > nit: drop Done http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/master.h File src/kudu/master/master.h: http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/master.h@92 PS5, Line 92: MasterOptions* mutable_opts() { return &opts_; } > Is this needed still? Good catch. http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/master.cc File src/kudu/master/master.cc: http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/master.cc@64 PS5, Line 64: namespace kudu { : namespace rpc { : class RpcContext; : } // namespace rpc : } // namespace kudu > nit: can you move this down into the other kudu namespace? Done http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/master_service.cc@267 PS5, Line 267: InitiateMasterChange > nit: InitiateMasterChangeConfig 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: 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: Wed, 23 Sep 2020 16:31:17 +0000 Gerrit-HasComments: Yes
