You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2017/07/28 00:12:30 UTC

[kudu-CR] consensus: switch RaftConsensus to shared ptr 

Hello David Ribeiro Alves, Mike Percy, Todd Lipcon,

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

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

to review the following change.

Change subject: consensus: switch RaftConsensus to shared_ptr
......................................................................

consensus: switch RaftConsensus to shared_ptr

Using shared_ptr instead of scoped_refptr means we can create weak pointers
to RaftConsensus, and I'd like to take advantage of that in future work.

The change is largely mechanical. The interesting part is the corresponding
switch from kudu::{Bind,Callback} to std::{bind,function}. To maintain
equivalent ownership semantics, the following conversions to bound arguments
are needed:
- Unretained(this) -> this
- this -> shared_from_this()

I also took the liberty of converting several pass-by-cref functions to use
move semantics.

Change-Id: I7fcaf5b7e4c4ce19126972fa0a81764b7da34e48
---
M src/kudu/consensus/consensus-test-util.h
M src/kudu/consensus/leader_election-test.cc
M src/kudu/consensus/leader_election.cc
M src/kudu/consensus/leader_election.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/master/catalog_manager.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/tserver-path-handlers.cc
M src/kudu/util/async_util.h
M src/kudu/util/status_callback.h
18 files changed, 205 insertions(+), 138 deletions(-)


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

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

[kudu-CR] consensus: switch RaftConsensus to shared ptr

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

Change subject: consensus: switch RaftConsensus to shared_ptr
......................................................................


consensus: switch RaftConsensus to shared_ptr

Using shared_ptr instead of scoped_refptr means we can create weak pointers
to RaftConsensus, and I'd like to take advantage of that in future work.

The change is largely mechanical. The interesting part is the corresponding
switch from kudu::{Bind,Callback} to std::{bind,function}. To maintain
equivalent ownership semantics, the following conversions to bound arguments
are needed:
- Unretained(this) -> this
- this -> shared_from_this()

I also took the liberty of converting several pass-by-cref functions to use
move semantics.

Change-Id: I7fcaf5b7e4c4ce19126972fa0a81764b7da34e48
Reviewed-on: http://gerrit.cloudera.org:8080/7531
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/consensus/consensus-test-util.h
M src/kudu/consensus/leader_election-test.cc
M src/kudu/consensus/leader_election.cc
M src/kudu/consensus/leader_election.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/master/catalog_manager.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/tserver-path-handlers.cc
M src/kudu/util/async_util.h
M src/kudu/util/status_callback.h
18 files changed, 208 insertions(+), 138 deletions(-)

Approvals:
  Todd Lipcon: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I7fcaf5b7e4c4ce19126972fa0a81764b7da34e48
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] consensus: switch RaftConsensus to shared ptr

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

Change subject: consensus: switch RaftConsensus to shared_ptr
......................................................................


Patch Set 1:

(1 comment)

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

PS1, Line 2674: replicated_cb_ == nullptr
I think just using the implicit boolean cast here (PREDICT_FALSE(!replicated_cb_)) is a little clearer than the comparison with nullptr. I see from the docs that they're equivalent, but this briefly confused me because I thought "huh? it's not a pointer!"


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fcaf5b7e4c4ce19126972fa0a81764b7da34e48
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] consensus: switch RaftConsensus to shared ptr

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

Change subject: consensus: switch RaftConsensus to shared_ptr
......................................................................


Patch Set 1:

(2 comments)

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

PS1, Line 2674: replicated_cb_ == nullptr
> I think just using the implicit boolean cast here (PREDICT_FALSE(!replicate
I think I did this because I had !PREDICT_FALSE(replicated_cb_) in the beginning, which didn't compile. Moving the negation into the PREDICT_FALSE(), however, is fine.


http://gerrit.cloudera.org:8080/#/c/7531/1/src/kudu/consensus/raft_consensus.h
File src/kudu/consensus/raft_consensus.h:

Line 387:   Status ReplicateConfigChangeUnlocked(
> Extra space?
Not sure what you mean. If you're referring to the indentation, it's typical to indent continuation lines with 4 characters like this.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fcaf5b7e4c4ce19126972fa0a81764b7da34e48
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] consensus: switch RaftConsensus to shared ptr

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

Change subject: consensus: switch RaftConsensus to shared_ptr
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7531/1/src/kudu/consensus/raft_consensus.h
File src/kudu/consensus/raft_consensus.h:

Line 387:   Status ReplicateConfigChangeUnlocked(
> Not sure what you mean. If you're referring to the indentation, it's typica
Yeah, I mean the indentation. Thanks for the clarification. I just thought the length of the line is a little short. And may not necessarily need the indentation. But I am good with both ways.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fcaf5b7e4c4ce19126972fa0a81764b7da34e48
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] consensus: switch RaftConsensus to shared ptr

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

Change subject: consensus: switch RaftConsensus to shared_ptr
......................................................................


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fcaf5b7e4c4ce19126972fa0a81764b7da34e48
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] consensus: switch RaftConsensus to shared ptr

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

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

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

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

Change subject: consensus: switch RaftConsensus to shared_ptr
......................................................................

consensus: switch RaftConsensus to shared_ptr

Using shared_ptr instead of scoped_refptr means we can create weak pointers
to RaftConsensus, and I'd like to take advantage of that in future work.

The change is largely mechanical. The interesting part is the corresponding
switch from kudu::{Bind,Callback} to std::{bind,function}. To maintain
equivalent ownership semantics, the following conversions to bound arguments
are needed:
- Unretained(this) -> this
- this -> shared_from_this()

I also took the liberty of converting several pass-by-cref functions to use
move semantics.

Change-Id: I7fcaf5b7e4c4ce19126972fa0a81764b7da34e48
---
M src/kudu/consensus/consensus-test-util.h
M src/kudu/consensus/leader_election-test.cc
M src/kudu/consensus/leader_election.cc
M src/kudu/consensus/leader_election.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/master/catalog_manager.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tserver/tablet_copy_source_session.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/tserver-path-handlers.cc
M src/kudu/util/async_util.h
M src/kudu/util/status_callback.h
18 files changed, 208 insertions(+), 138 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7fcaf5b7e4c4ce19126972fa0a81764b7da34e48
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] consensus: switch RaftConsensus to shared ptr

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

Change subject: consensus: switch RaftConsensus to shared_ptr
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7531/1/src/kudu/consensus/raft_consensus.h
File src/kudu/consensus/raft_consensus.h:

Line 387:   Status ReplicateConfigChangeUnlocked(
Extra space?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fcaf5b7e4c4ce19126972fa0a81764b7da34e48
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes