Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17126 )
Change subject: [test][master] KUDU-3249 Recover dead master at the same HostPort ...................................................................... Patch Set 1: Code-Review+1 (4 comments) http://gerrit.cloudera.org:8080/#/c/17126/1/src/kudu/master/dynamic_multi_master-test.cc File src/kudu/master/dynamic_multi_master-test.cc: http://gerrit.cloudera.org:8080/#/c/17126/1/src/kudu/master/dynamic_multi_master-test.cc@643 PS1, Line 643: ASSERT_OK(CreateTable(cluster_.get(), kTableName)); nit: this was surprising to see. Is it supposed to be here? It's not mentioned in the method comment, and there's no other context comment nearby explaining why it's important. Also, consider putting it in the tests themselves rather in this helper method. http://gerrit.cloudera.org:8080/#/c/17126/1/src/kudu/master/dynamic_multi_master-test.cc@662 PS1, Line 662: Before removing the master set the output parameters as the master being removed : // will not be tracked by ExternalMiniCluster. nit: these values don't change though, so what's the importance of setting them here? http://gerrit.cloudera.org:8080/#/c/17126/1/src/kudu/master/dynamic_multi_master-test.cc@701 PS1, Line 701: consensus:: nit: given how frequently this is used, add a "using" declaration http://gerrit.cloudera.org:8080/#/c/17126/1/src/kudu/master/dynamic_multi_master-test.cc@1079 PS1, Line 1079: Please follow the next steps which includes system catalog " : "tablet copy" nit: missing a $0 somewhere? -- To view, visit http://gerrit.cloudera.org:8080/17126 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iac65201fd39ede5e918025ca7cf6a852a92d6eec Gerrit-Change-Number: 17126 Gerrit-PatchSet: 1 Gerrit-Owner: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 02 Mar 2021 19:42:27 +0000 Gerrit-HasComments: Yes
