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/08/01 00:18:08 UTC
[kudu-CR] consensus peers: capture weak refs in functors submitted to thread pools
Hello Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/7543
to review the following change.
Change subject: consensus_peers: capture weak refs in functors submitted to thread pools
......................................................................
consensus_peers: capture weak refs in functors submitted to thread pools
By capturing weak refs we allow peers to be destroyed earlier, closer to
last-user-facing-ref-was-dropped time. While this reduces the amount of
"unproductive" submitted tasks processed, I think the real benefit is a
clearer expression of lifecycle: we capture weak refs where we do not wish
to prolong the Peer's lifespan.
Note: this change is less useful than I had hoped due to the continued
strong ref capture in async proxy calls. Weak refs don't work there as the
krpc subsystem requires response objects to stay alive right up until the
supplied callback is invoked.
Change-Id: I6dbb777e7e141f1f5bf48026779f1d4b26185195
---
M src/kudu/consensus/consensus_peers.cc
M src/kudu/consensus/consensus_peers.h
2 files changed, 40 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/7543/1
--
To view, visit http://gerrit.cloudera.org:8080/7543
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6dbb777e7e141f1f5bf48026779f1d4b26185195
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] consensus peers: capture weak refs in functors submitted to thread pools
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.
Change subject: consensus_peers: capture weak refs in functors submitted to thread pools
......................................................................
consensus_peers: capture weak refs in functors submitted to thread pools
By capturing weak refs we allow peers to be destroyed earlier, closer to
last-user-facing-ref-was-dropped time. While this reduces the amount of
"unproductive" submitted tasks processed, I think the real benefit is a
clearer expression of lifecycle: we capture weak refs where we do not wish
to prolong the Peer's lifespan.
Note: this change is less useful than I had hoped due to the continued
strong ref capture in async proxy calls. Weak refs don't work there as the
krpc subsystem requires response objects to stay alive right up until the
supplied callback is invoked.
Change-Id: I6dbb777e7e141f1f5bf48026779f1d4b26185195
Reviewed-on: http://gerrit.cloudera.org:8080/7543
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Kudu Jenkins
---
M src/kudu/consensus/consensus_peers.cc
M src/kudu/consensus/consensus_peers.h
2 files changed, 32 insertions(+), 28 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/7543
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I6dbb777e7e141f1f5bf48026779f1d4b26185195
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] consensus peers: capture weak refs in functors submitted to thread pools
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/7543
to look at the new patch set (#3).
Change subject: consensus_peers: capture weak refs in functors submitted to thread pools
......................................................................
consensus_peers: capture weak refs in functors submitted to thread pools
By capturing weak refs we allow peers to be destroyed earlier, closer to
last-user-facing-ref-was-dropped time. While this reduces the amount of
"unproductive" submitted tasks processed, I think the real benefit is a
clearer expression of lifecycle: we capture weak refs where we do not wish
to prolong the Peer's lifespan.
Note: this change is less useful than I had hoped due to the continued
strong ref capture in async proxy calls. Weak refs don't work there as the
krpc subsystem requires response objects to stay alive right up until the
supplied callback is invoked.
Change-Id: I6dbb777e7e141f1f5bf48026779f1d4b26185195
---
M src/kudu/consensus/consensus_peers.cc
M src/kudu/consensus/consensus_peers.h
2 files changed, 32 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/7543/3
--
To view, visit http://gerrit.cloudera.org:8080/7543
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6dbb777e7e141f1f5bf48026779f1d4b26185195
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: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] consensus peers: capture weak refs in functors submitted to thread pools
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: consensus_peers: capture weak refs in functors submitted to thread pools
......................................................................
Patch Set 3: Code-Review+2
Maybe worth looping raft_consensus-itest a bunch before committing if you have a few minutes.
--
To view, visit http://gerrit.cloudera.org:8080/7543
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6dbb777e7e141f1f5bf48026779f1d4b26185195
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: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] consensus peers: capture weak refs in functors submitted to thread pools
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: consensus_peers: capture weak refs in functors submitted to thread pools
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7543/2//COMMIT_MSG
Commit Message:
PS2, Line 15: less useful than I had hoped
does it have any use at all then? or is it just documentation value? ie is the lifespan actually changing based on this patch?
http://gerrit.cloudera.org:8080/#/c/7543/2/src/kudu/consensus/consensus_peers.cc
File src/kudu/consensus/consensus_peers.cc:
PS2, Line 150: shared_ptr<Peer> p = w_this.lock();
: if (!p) {
: // The functor outlived its peer.
: return;
: }
: p->SendNextRequest(even_if_queue_empty);
I seem to recall that in newer versions of C++ you can do:
if (auto p = w_this.lock()) {
p->SendNextRequest(...);
}
do you think that reads better?
--
To view, visit http://gerrit.cloudera.org:8080/7543
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6dbb777e7e141f1f5bf48026779f1d4b26185195
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@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 peers: capture weak refs in functors submitted to thread pools
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: consensus_peers: capture weak refs in functors submitted to thread pools
......................................................................
Patch Set 3:
> Maybe worth looping raft_consensus-itest a bunch before committing
> if you have a few minutes.
I looped it 1000 times. Wound up with a couple instances of KUDU-2088 as well as a few other unusual errors which can be seen here: https://gist.github.com/adembo/3c92e33e9969974b1f02be13f23bdcc4.
I don't believe these errors were due to this patch as lifecycle issues typically manifest as crashes. So I'm going to merge this.
--
To view, visit http://gerrit.cloudera.org:8080/7543
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6dbb777e7e141f1f5bf48026779f1d4b26185195
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: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] consensus peers: capture weak refs in functors submitted to thread pools
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: consensus_peers: capture weak refs in functors submitted to thread pools
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7543/2//COMMIT_MSG
Commit Message:
PS2, Line 15: less useful than I had hoped
> does it have any use at all then? or is it just documentation value? ie is
The lifespan does shrink if there are no outstanding RPCs; this text just reflects my earlier hope of doing "weak ref on every capture" rather than "weak ref on every capture if possible, strong ref otherwise".
http://gerrit.cloudera.org:8080/#/c/7543/2/src/kudu/consensus/consensus_peers.cc
File src/kudu/consensus/consensus_peers.cc:
PS2, Line 150: shared_ptr<Peer> p = w_this.lock();
: if (!p) {
: // The functor outlived its peer.
: return;
: }
: p->SendNextRequest(even_if_queue_empty);
> I seem to recall that in newer versions of C++ you can do:
Yes, that's pretty nice. Will use that instead.
--
To view, visit http://gerrit.cloudera.org:8080/7543
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6dbb777e7e141f1f5bf48026779f1d4b26185195
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: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes