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/05/22 22:01:45 UTC

[kudu-CR] KUDU-871 (part 2). Make ConsensusMetadata thread-safe 

Hello David Ribeiro Alves, Todd Lipcon,

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

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

to review the following change.

Change subject: KUDU-871 (part 2). Make ConsensusMetadata thread-safe
......................................................................

KUDU-871 (part 2). Make ConsensusMetadata thread-safe

This patch simply adds locks around all accessors and makes
ConsensusMetadata refcounted. This enabled ConsensusMetadata to be a
long-lived shared object that brokers access to the underlying
ConsensusMetadata file in a followup patch.

TODO: Could use an MT test in consensus_meta-test to exercise the lock.

Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
---
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/consensus/raft_consensus_state.cc
M src/kudu/consensus/raft_consensus_state.h
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
22 files changed, 195 insertions(+), 98 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Make ConsensusMetadata thread-safe

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.

Change subject: Make ConsensusMetadata thread-safe
......................................................................


Patch Set 3: Code-Review+2

(1 comment)

feel free to address the nit or not and re+2

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

PS3, Line 181: below mutable fields.
xs nit: "mutable fields below"?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Make ConsensusMetadata thread-safe

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: Make ConsensusMetadata thread-safe
......................................................................


Patch Set 4: Verified+1

overriding flaky test

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Make ConsensusMetadata thread-safe

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: Make ConsensusMetadata thread-safe
......................................................................


Patch Set 10: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Make ConsensusMetadata thread-safe

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: Make ConsensusMetadata thread-safe
......................................................................


Patch Set 6: Code-Review-1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Make ConsensusMetadata thread-safe

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Todd Lipcon,

I'd like you to reexamine a change.  Please visit

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

to look at the new patch set (#10).

Change subject: Make ConsensusMetadata thread-safe
......................................................................

Make ConsensusMetadata thread-safe

This patch simply adds locks around all accessors and makes
ConsensusMetadata refcounted. This enables ConsensusMetadata to be
safely used outside of the RaftConsensus class (with certain caveats).

This is needed to implement tombstoned voting in a follow-up patch.

Also, add some helper methods to ConsensusMetadata to avoid making
excessive copies of the RaftConfigPB in various cases:

  bool IsVoterInConfig(const std::string& uuid, RaftConfigState type);
  bool IsMemberInConfig(const std::string& uuid, RaftConfigState type);
  int CountVotersInConfig(RaftConfigState type);
  int64_t GetConfigOpIdIndex(RaftConfigState type);

Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
---
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
M src/kudu/consensus/quorum_util.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
18 files changed, 315 insertions(+), 157 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/6958/10
-- 
To view, visit http://gerrit.cloudera.org:8080/6958
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Make ConsensusMetadata thread-safe

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged.

Change subject: Make ConsensusMetadata thread-safe
......................................................................


Make ConsensusMetadata thread-safe

This patch simply adds locks around all accessors and makes
ConsensusMetadata refcounted. This enables ConsensusMetadata to be
safely used outside of the RaftConsensus class (with certain caveats).

This is needed to implement tombstoned voting in a follow-up patch.

Also, add some helper methods to ConsensusMetadata to avoid making
excessive copies of the RaftConfigPB in various cases:

  bool IsVoterInConfig(const std::string& uuid, RaftConfigState type);
  bool IsMemberInConfig(const std::string& uuid, RaftConfigState type);
  int CountVotersInConfig(RaftConfigState type);
  int64_t GetConfigOpIdIndex(RaftConfigState type);

Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Reviewed-on: http://gerrit.cloudera.org:8080/6958
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Mike Percy <mp...@apache.org>
---
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
M src/kudu/consensus/quorum_util.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
18 files changed, 315 insertions(+), 157 deletions(-)

Approvals:
  Mike Percy: Verified
  Todd Lipcon: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-871 (part 2). Make ConsensusMetadata thread-safe

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: KUDU-871 (part 2). Make ConsensusMetadata thread-safe
......................................................................


Patch Set 1: Verified+1

I can't reproduce this failure (even after 1000s of iters on dist-test with different levels of stress / build types)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Make ConsensusMetadata thread-safe

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: Make ConsensusMetadata thread-safe
......................................................................


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/6958/6/src/kudu/consensus/consensus_meta.h
File src/kudu/consensus/consensus_meta.h:

Line 99:   RaftConfigPB committed_config() const;
> If this returns a copy of a heavy-weight PB it should be renamed to Committ
OK, I went through and removed all of the hot-path copies that I found. Also renamed the copying methods to InitialCaps convention to make them more obviously copying.


Line 106:   RaftConfigPB pending_config() const;
> same
Done


Line 114:   RaftConfigPB active_config() const;
> same
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Make ConsensusMetadata thread-safe

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

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

to look at the new patch set (#3).

Change subject: Make ConsensusMetadata thread-safe
......................................................................

Make ConsensusMetadata thread-safe

This patch simply adds locks around all accessors and makes
ConsensusMetadata refcounted. This enables ConsensusMetadata to be
safely used outside of the RaftConsensus class (with certain caveats).

This is needed to implement tombstoned voting in a follow-up patch.

Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
---
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
17 files changed, 199 insertions(+), 102 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/6958/3
-- 
To view, visit http://gerrit.cloudera.org:8080/6958
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Make ConsensusMetadata thread-safe

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: Make ConsensusMetadata thread-safe
......................................................................


Patch Set 9: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Make ConsensusMetadata thread-safe

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Alexey Serbin,

I'd like you to reexamine a change.  Please visit

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

to look at the new patch set (#2).

Change subject: Make ConsensusMetadata thread-safe
......................................................................

Make ConsensusMetadata thread-safe

This patch simply adds locks around all accessors and makes
ConsensusMetadata refcounted. This enables ConsensusMetadata to be
safely used outside of the RaftConsensus class (with certain caveats).

This is needed to implement tombstoned voting in a follow-up patch.

Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
---
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
17 files changed, 198 insertions(+), 101 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/6958/2
-- 
To view, visit http://gerrit.cloudera.org:8080/6958
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-871 (part 2). Make ConsensusMetadata thread-safe

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: KUDU-871 (part 2). Make ConsensusMetadata thread-safe
......................................................................


Patch Set 1:

> I haven't looked at the implementation here yet but I'm wondering
 > whether this may have potential performance impact.
 > 
 > In particular, we would hold this lock while flushing/fsyncing new
 > ConsensusMetadata to disk. Are there code paths that previously
 > would not block but now block? Or were we always previously
 > protected by the ReplicaState lock, and now we're protected by the
 > ConsensusMeta lock?

No, they are the same. I don't think there will be much of a performance impact because the vast majority of the time we will be acquiring an uncontended futex due to holding the ReplicaState lock to do things like get the current term in RaftConsensus / ReplicaState.

 > Regarding the above blocking (which is already problematic) -- does
 > this get us closer or farther away from an end state where the
 > cmeta is synchronized using an RWCLock? It would be great to allow
 > read-only callers to continue to read while an fsync is going on,
 > for example.

I like this idea but I hadn't factored it in when writing this. I don't think it really helps or hurts us from the perspective of doing that. However it's likely we'd need a significant refactor of ReplicaState to do that (which we should do anyway).

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Make ConsensusMetadata thread-safe

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.

Change subject: Make ConsensusMetadata thread-safe
......................................................................


Patch Set 6: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Make ConsensusMetadata thread-safe

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: Make ConsensusMetadata thread-safe
......................................................................


Patch Set 10: Verified+1

overriding flaky test

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Make ConsensusMetadata thread-safe

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Todd Lipcon,

I'd like you to reexamine a change.  Please visit

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

to look at the new patch set (#8).

Change subject: Make ConsensusMetadata thread-safe
......................................................................

Make ConsensusMetadata thread-safe

This patch simply adds locks around all accessors and makes
ConsensusMetadata refcounted. This enables ConsensusMetadata to be
safely used outside of the RaftConsensus class (with certain caveats).

This is needed to implement tombstoned voting in a follow-up patch.

Also, add some helper methods to ConsensusMetadata to avoid making
excessive copies of the RaftConfigPB in various cases:

  bool IsVoterInConfig(const std::string& uuid, RaftConfigState type);
  bool IsMemberInConfig(const std::string& uuid, RaftConfigState type);
  int CountVotersInConfig(RaftConfigState type);
  int64_t GetConfigOpIdIndex(RaftConfigState type);

Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
---
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
M src/kudu/consensus/quorum_util.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
18 files changed, 335 insertions(+), 147 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/6958/8
-- 
To view, visit http://gerrit.cloudera.org:8080/6958
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Make ConsensusMetadata thread-safe

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Todd Lipcon,

I'd like you to reexamine a change.  Please visit

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

to look at the new patch set (#7).

Change subject: Make ConsensusMetadata thread-safe
......................................................................

Make ConsensusMetadata thread-safe

This patch simply adds locks around all accessors and makes
ConsensusMetadata refcounted. This enables ConsensusMetadata to be
safely used outside of the RaftConsensus class (with certain caveats).

This is needed to implement tombstoned voting in a follow-up patch.

Also, add some helper methods to ConsensusMetadata to avoid making
excessive copies of the RaftConfigPB in various cases:

  bool IsVoterInConfig(const std::string& uuid, RaftConfigState type);
  bool IsMemberInConfig(const std::string& uuid, RaftConfigState type);
  int CountVotersInConfig(RaftConfigState type);
  int64_t GetConfigOpIdIndex(RaftConfigState type);

Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
---
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
M src/kudu/consensus/quorum_util.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
18 files changed, 338 insertions(+), 147 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/6958/7
-- 
To view, visit http://gerrit.cloudera.org:8080/6958
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-871 (part 2). Make ConsensusMetadata thread-safe

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: KUDU-871 (part 2). Make ConsensusMetadata thread-safe
......................................................................


Patch Set 1:

I haven't looked at the implementation here yet but I'm wondering whether this may have potential performance impact.

In particular, we would hold this lock while flushing/fsyncing new ConsensusMetadata to disk. Are there code paths that previously would not block but now block? Or were we always previously protected by the ReplicaState lock, and now we're protected by the ConsensusMeta lock?

Regarding the above blocking (which is already problematic) -- does this get us closer or farther away from an end state where the cmeta is synchronized using an RWCLock? It would be great to allow read-only callers to continue to read while an fsync is going on, for example.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Make ConsensusMetadata thread-safe

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: Make ConsensusMetadata thread-safe
......................................................................


Patch Set 9:

(3 comments)

just a few small items

http://gerrit.cloudera.org:8080/#/c/6958/9/src/kudu/consensus/consensus_meta.cc
File src/kudu/consensus/consensus_meta.cc:

PS9, Line 135:   lock_guard<Mutex> l(lock_);
             :   switch (type) {
             :     case ACTIVE_CONFIG:
             :       return IsRaftConfigVoter(uuid, active_config_unlocked());
             :     case COMMITTED_CONFIG:
             :       return IsRaftConfigVoter(uuid, committed_config_unlocked());
             :     case PENDING_CONFIG:
             :       return IsRaftConfigVoter(uuid, pending_config_unlocked());
             :     default:
             :       LOG(FATAL) << "Unsupported RaftConfigState: " << type;
             :   }
could this and the three functions below be simpler if you added something like:

const RaftConfigPB& config_unlocked(RaftConfigState type) {
  switch (type) {
    case ACTIVE_CONFIG: return active_config_unlocked();
    case COMMITTED_CONFIG: return committed_config_unlocked();
    case PENDING_COPNFIG: return pending_config_unlocked();
  }
}

bool IsVoterInConfig(const string& uuid, RaftConfigState type) {
  lock_guard<Mutex> l(lock_);
  return IsRaftConfigVoter(uuid, config_unlocked(type));
}

...


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

Line 841: Status RaftConsensus::IsSingleVoterConfig(bool* single_voter) const {
mind adding a TODO here that there's no reason for this to return a Status? should just be a normal getter returning bool


Line 2345:   int64_t committed_config_opid_index = cmeta_->GetConfigOpIdIndex(COMMITTED_CONFIG);
isn't this equivalent to committed_config.opid_index() now? seems unnecessary


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Make ConsensusMetadata thread-safe

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: Make ConsensusMetadata thread-safe
......................................................................


Patch Set 6: Verified+1

overriding flaky test

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Make ConsensusMetadata thread-safe

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: Make ConsensusMetadata thread-safe
......................................................................


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/6958/6/src/kudu/consensus/consensus_meta.h
File src/kudu/consensus/consensus_meta.h:

Line 99:   RaftConfigPB committed_config() const;
If this returns a copy of a heavy-weight PB it should be renamed to CommittedConfig.

More broadly, I'm concerned that this may actually be a perf issue. For example see e14b82496da992c40af3fbf2c24a97f38b1d96cc in which I changed to using 'cmeta_->active_config()' specifically to avoid reallocating/copying the RaftConfigPB. I think this will regress that change and potentially introduce other problematic cases.

Not sure if that's the only case of a hot-path call into this but the PBs are pretty heavy-weight to copy on every UpdateConsensus call. (I think it's fine if they get copied during voting/election/etc since those are rare)


Line 106:   RaftConfigPB pending_config() const;
same


Line 114:   RaftConfigPB active_config() const;
same


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Make ConsensusMetadata thread-safe

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: Make ConsensusMetadata thread-safe
......................................................................


Patch Set 9:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/6958/9/src/kudu/consensus/consensus_meta.cc
File src/kudu/consensus/consensus_meta.cc:

PS9, Line 135:   lock_guard<Mutex> l(lock_);
             :   switch (type) {
             :     case ACTIVE_CONFIG:
             :       return IsRaftConfigVoter(uuid, active_config_unlocked());
             :     case COMMITTED_CONFIG:
             :       return IsRaftConfigVoter(uuid, committed_config_unlocked());
             :     case PENDING_CONFIG:
             :       return IsRaftConfigVoter(uuid, pending_config_unlocked());
             :     default:
             :       LOG(FATAL) << "Unsupported RaftConfigState: " << type;
             :   }
> could this and the three functions below be simpler if you added something 
Good idea. Done.


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

Line 841: Status RaftConsensus::IsSingleVoterConfig(bool* single_voter) const {
> mind adding a TODO here that there's no reason for this to return a Status?
I just fixed the signature


Line 2345:   int64_t committed_config_opid_index = cmeta_->GetConfigOpIdIndex(COMMITTED_CONFIG);
> isn't this equivalent to committed_config.opid_index() now? seems unnecessa
ah, yeah, I meant to remove line 2344


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Make ConsensusMetadata thread-safe

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.

Change subject: Make ConsensusMetadata thread-safe
......................................................................


Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Make ConsensusMetadata thread-safe

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: Make ConsensusMetadata thread-safe
......................................................................


Patch Set 3:

(1 comment)

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

PS3, Line 181: below mutable fields.
> xs nit: "mutable fields below"?
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Make ConsensusMetadata thread-safe

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

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

to look at the new patch set (#4).

Change subject: Make ConsensusMetadata thread-safe
......................................................................

Make ConsensusMetadata thread-safe

This patch simply adds locks around all accessors and makes
ConsensusMetadata refcounted. This enables ConsensusMetadata to be
safely used outside of the RaftConsensus class (with certain caveats).

This is needed to implement tombstoned voting in a follow-up patch.

Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
---
M src/kudu/consensus/consensus_meta-test.cc
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/consensus/raft_consensus_quorum-test.cc
M src/kudu/integration-tests/ts_recovery-itest.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tools/tool_action_local_replica.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
17 files changed, 199 insertions(+), 102 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/58/6958/4
-- 
To view, visit http://gerrit.cloudera.org:8080/6958
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id2c70605c0593e55486184705ea448dcb4bef2d7
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>