Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16936 )
Change subject: [master] KUDU-2181 Raft ChangeConfig request to remove a master ...................................................................... Patch Set 6: Code-Review+2 (2 comments) Overall looks good to me, just a few nits. http://gerrit.cloudera.org:8080/#/c/16936/6/src/kudu/master/dynamic_multi_master-test.cc File src/kudu/master/dynamic_multi_master-test.cc: http://gerrit.cloudera.org:8080/#/c/16936/6/src/kudu/master/dynamic_multi_master-test.cc@1013 PS6, Line 1013: // Prevent master leadership transfer for a deterministic test. : for (int i = 0 ; i < cluster_->num_masters(); i++) { : // Starting the cluster with following flag leads to a case sometimes : // wherein no leader gets elected leading to failure in ConnectToMaster() RPC. : // So instead set the flag after the cluster is running. : ASSERT_OK(cluster_->SetFlag(cluster_->master(i), "leader_failure_max_missed_heartbeat_periods", : "10.0")); : } nit for here and for TestRemoveLeaderMaster scenario: does it make sense to separate this into a sub-routine/function? http://gerrit.cloudera.org:8080/#/c/16936/6/src/kudu/master/dynamic_multi_master-test.cc@1082 PS6, Line 1082: // ASSERT_EVENTUALLY to retry in case of master leadership transfer. : ASSERT_EVENTUALLY([&] { nit for here and elsewhere where ASSERT_EVENTUALLY added from PS5 to PS6: would it help to use the trick with setting --leader_failure_max_missed_heartbeat_periods to higher values instead? I guess it would help to understand why this approach is preferable vs bumping up --leader_failure_max_missed_heartbeat_periods: maybe, add a comment to explain why the approach with ASSERT_EVENTUALLY is a better fit for some cases? -- To view, visit http://gerrit.cloudera.org:8080/16936 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I76c03b8850faef60b65f85184c0a4db7cc6759ee Gerrit-Change-Number: 16936 Gerrit-PatchSet: 6 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-Comment-Date: Sun, 07 Feb 2021 01:29:33 +0000 Gerrit-HasComments: Yes
