You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2017/11/22 23:52:18 UTC

[kudu-CR] WIP: KUDU-1097 (patch 3): Implement promotion of NON VOTER replicas

Hello Alexey Serbin,

I'd like you to do a code review. Please visit

    http://gerrit.cloudera.org:8080/8633

to review the following change.


Change subject: WIP: KUDU-1097 (patch 3): Implement promotion of NON_VOTER replicas
......................................................................

WIP: KUDU-1097 (patch 3): Implement promotion of NON_VOTER replicas

TODO: needs tests

Change-Id: Ife6f59658cb2f38d087d57c244ad811010a91fef
---
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/consensus_queue.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/integration-tests/raft_config_change-itest.cc
5 files changed, 171 insertions(+), 1 deletion(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/33/8633/1
-- 
To view, visit http://gerrit.cloudera.org:8080/8633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ife6f59658cb2f38d087d57c244ad811010a91fef
Gerrit-Change-Number: 8633
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>

[kudu-CR] KUDU-1097 (patch 3): Implement promotion of NON VOTER replicas

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8633 )

Change subject: KUDU-1097 (patch 3): Implement promotion of NON_VOTER replicas
......................................................................


Patch Set 5: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/8633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife6f59658cb2f38d087d57c244ad811010a91fef
Gerrit-Change-Number: 8633
Gerrit-PatchSet: 5
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 27 Nov 2017 07:26:40 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-1097 (patch 3): Implement promotion of NON VOTER replicas

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8633 )

Change subject: KUDU-1097 (patch 3): Implement promotion of NON_VOTER replicas
......................................................................


Patch Set 3:

(14 comments)

looks good overall, some nits

http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/consensus/consensus_queue.h
File src/kudu/consensus/consensus_queue.h:

http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/consensus/consensus_queue.h@553
PS3, Line 553:   virtual void NotifyPeerToPromote(const std::string& peer_uuid,
nit: add comment/doc?


http://gerrit.cloudera.org:8080/#/c/8633/2/src/kudu/consensus/consensus_queue.cc
File src/kudu/consensus/consensus_queue.cc:

http://gerrit.cloudera.org:8080/#/c/8633/2/src/kudu/consensus/consensus_queue.cc@909
PS2, Line 909: Factor this out into a dedicated function
indeed


http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/consensus/consensus_queue.cc
File src/kudu/consensus/consensus_queue.cc:

http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/consensus/consensus_queue.cc@1308
PS3, Line 1308: // TODO(mpercy): FIXME
Could you be more specific on what's is wrong here?


http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/consensus/raft_consensus.cc@817
PS3, Line 817: Notified about a promotion requested for $0: "
nit: may be just

"attempt to promote $0"


http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/consensus/raft_consensus.cc@823
PS3, Line 823: msg << "Requested in "
             :                                      << "previous term " << term
             :                                      << ", but a leader election "
             :                                      << "likely occurred since then. Doing nothing.";
maybe just:

Substitute("$0: current term ($1) is different from the term of request ($2), aborting", msg, term, current_term)


http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/consensus/raft_consensus.cc@833
PS3, Line 833: msg << "Notified when "
             :                                      << "committed config had opid index "
             :                                      << committed_config_opid_index << ", but now "
             :                                      << "the committed config has opid index "
             :                                      << current_committed_config_opid_index
             :                                      << ". Doing nothing."
maybe just:

Substitute("$0: config opid index mismatch: requested at $1, current committed $2, aborting", msg, committed_config_opid_index, current_committed_config_opid_index);


http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/consensus/raft_consensus.cc@843
PS3, Line 843: msg << "There is already a config change operation "
             :                                      << "in progress. Unable to promote follower until it "
             :                                      << "completes. Doing nothing.";
maybe just:

Substitute("$0: cannot promote non-voter when another config change operation in progress, aborting");


http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/consensus/raft_consensus.cc@1741
PS3, Line 1741: handle a non-voter that had both its
              :         // "promote" and "replace" flags set.
I'm not sure this is ever needed at all.

But so far the semantics of non-clobbering not-specified attributes looks good to me.


http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/integration-tests/cluster_itest_util.h
File src/kudu/integration-tests/cluster_itest_util.h:

http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/integration-tests/cluster_itest_util.h@279
PS3, Line 279: consensus::RaftPeerAttrsPB()
nit: would {} be sufficient here instead of consensus::RaftPeerAttrsPB() ?


http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/integration-tests/raft_config_change-itest.cc
File src/kudu/integration-tests/raft_config_change-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/integration-tests/raft_config_change-itest.cc@170
PS3, Line 170: 3
nit:

I think it would be more expressive to set corresponding gflag  prior to that:

constexpr int kNumReplicas = 3;
FLAGS_num_replicas = kNumReplicas;

ASSERT_OK(inspect_->WaitForReplicaCount(kNumReplicas));


http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/integration-tests/raft_config_change-itest.cc@194
PS3, Line 194: LOG(INFO) << "Looking for tablet leader...";
As for this and other LOG(INFO) -- is it crucial to have those?


http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/integration-tests/raft_config_change-itest.cc@211
PS3, Line 211: }
nit: consider adding

NO_FATALS(cluster_->AssertNoCrashes());


http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/integration-tests/raft_consensus-itest.cc
File src/kudu/integration-tests/raft_consensus-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/integration-tests/raft_consensus-itest.cc@1536
PS3, Line 1536: RaftPeerAttrsPB()
nit: would {} be sufficient here instead?


http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/integration-tests/raft_consensus-itest.cc@1544
PS3, Line 1544: RaftPeerAttrsPB()
ditto



-- 
To view, visit http://gerrit.cloudera.org:8080/8633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife6f59658cb2f38d087d57c244ad811010a91fef
Gerrit-Change-Number: 8633
Gerrit-PatchSet: 3
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 23 Nov 2017 03:08:43 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1097 (patch 3): Implement promotion of NON VOTER replicas

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8633 )

Change subject: KUDU-1097 (patch 3): Implement promotion of NON_VOTER replicas
......................................................................


Patch Set 4:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/consensus/raft_consensus.cc
File src/kudu/consensus/raft_consensus.cc:

http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/consensus/raft_consensus.cc@833
PS3, Line 833: msg << "notified when "
             :                                      << "committed config had opid index "
             :                                      << committed_config_opid_index << ", but now "
             :                                      << "the committed config has opid index "
             :                                      << current_committed_config_opid_index
             :                                      << ". Doing nothing."
> Again, I think the more verbose message I wrote will be easier for users to
It's fine with me.  I was just wanted to express the same amount of information in less words.  Also, code-wise, I would expect using Substitute() to be more readable than multi-line string with 'operator <<'


http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/consensus/raft_consensus.cc@843
PS3, Line 843: msg << "there is already a config change operation "
             :                                      << "in progress. Unable to promote follower until it "
             :                                      << "completes. Doing nothing.";
> I don't really see how what you wrote is an improvement over what is alread
It's more concise and readable: it delivers the same amount of actionable information, and it's less pollution in the log.

But if you prefer this longer message, it's fine with me.


http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/consensus/raft_consensus.cc@1741
PS3, Line 1741: handle a non-voter that had both its
              :         // "promote" and "replace" flags set.
> Do you think we should disallow it?
I'm saying the particular case of a non-voter with both 'replace' and 'promote' attributes set is not applicable in our current design, if I'm not mistaken.  So, maybe this particular part of the comment should be removed.

In general case, I think we should allow handling multiple attributes in the ChangeConfig request.


http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/integration-tests/raft_config_change-itest.cc
File src/kudu/integration-tests/raft_config_change-itest.cc:

http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/integration-tests/raft_config_change-itest.cc@170
PS3, Line 170: c
> Hmm, there are a couple things here:
Ah, sorry -- I thought RaftConfigChangeITest was a subclass of  TabletServerIntegrationTestBase.

The point with that update was to make sure we requested 3 replicas and we got 3 replicas running, not something like requested 5 replicas and got just 3 running.

If it's clear there are 3 replicas only, then it's fine as is then.


http://gerrit.cloudera.org:8080/#/c/8633/3/src/kudu/integration-tests/raft_config_change-itest.cc@194
PS3, Line 194: SERT_EVENTUALLY([&] {
> I just left this in for debugging in case this test ever fails, but I can r
sgtm: if it helps with debugging, it makes sense to leave those then.



-- 
To view, visit http://gerrit.cloudera.org:8080/8633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife6f59658cb2f38d087d57c244ad811010a91fef
Gerrit-Change-Number: 8633
Gerrit-PatchSet: 4
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 27 Nov 2017 07:26:04 +0000
Gerrit-HasComments: Yes