[kudu-CR] [master] KUDU-2181 Raft ChangeConfig request to remove a master

["Alexey Serbin (Code Review)" <ge...@cloudera.org>]

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 3: Code-Review+2

(5 comments)

Overall looks good to me, just a few nits.

http://gerrit.cloudera.org:8080/#/c/16936/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/16936/1//COMMIT_MSG@12
PS1, Line 12: tests
> That's a good point and I guess applies to both addition and removal of mas
Sure!  SGTM.


http://gerrit.cloudera.org:8080/#/c/16936/3/src/kudu/master/dynamic_multi_master-test.cc
File src/kudu/master/dynamic_multi_master-test.cc:

http://gerrit.cloudera.org:8080/#/c/16936/3/src/kudu/master/dynamic_multi_master-test.cc@253
PS3, Line 253: returns
nit: return?

Or change 'Verify' --> 'Verifies'


http://gerrit.cloudera.org:8080/#/c/16936/3/src/kudu/master/dynamic_multi_master-test.cc@950
PS3, Line 950:   ASSERT_TRUE(s.IsRemoteError());
nit here and elsewhere in similar ASSERT: just in case if it ever fails/triggers, consider outputting s.ToString() in such cases -- it helps with debugging; i.e. something like

ASSERT_TRUE(s.IsRemoteError()) << s.ToString();

Or make this assert to be EXPECT_TRUE(), so the following ASSERT_STR_MATCHES() would output more information on the error.

I guess the former choice if preferable because it's more universal.


http://gerrit.cloudera.org:8080/#/c/16936/3/src/kudu/master/master.cc
File src/kudu/master/master.cc:

http://gerrit.cloudera.org:8080/#/c/16936/3/src/kudu/master/master.cc@533
PS3, Line 533:  
nit: stick the ampersand to the type (i.e. to the 'auto')


http://gerrit.cloudera.org:8080/#/c/16936/3/src/kudu/master/master.cc@575
PS3, Line 575:         return Status::IllegalState(Substitute("Found multiple masters with same RPC address $0 "
             :                                                "and UUID $1", hp.ToString(), uuid));
I guess this might be changed into LOG(FATAL)?  First of, this should not be possible, and if it happened, how to get out of such a situation?  What do you think?



-- 
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: 3
Gerrit-Owner: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 01 Feb 2021 22:55:18 +0000
Gerrit-HasComments: Yes