Andrew Wong 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: (15 comments) Just nits for the most part. 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? 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 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. 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 them for use in a new master"? 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? 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 value should be. E.g. should it include the new master? 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 need to ASSERT_EVENTUALLY? 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. Could you get by using fewer of them, or comment why they are necessary at each usage? If we put too much into these blocks, they may hide some bugs. 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_masters + 1` where appropriate. That way it's obvious to readers no matter where in the test they are what the value is. 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? 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 the other negative tests? 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? 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? http://gerrit.cloudera.org:8080/#/c/16321/5/src/kudu/master/master.cc@424 PS5, Line 424: auto nit: could be a const ref 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 -- 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 19:47:17 +0000 Gerrit-HasComments: Yes
