You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2020/12/12 03:16:28 UTC
[kudu-CR] [rpc] weak ptr for Messenger in RpcRetrier
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16866
Change subject: [rpc] weak_ptr for Messenger in RpcRetrier
......................................................................
[rpc] weak_ptr for Messenger in RpcRetrier
This patch changes std::shared_ptr wrapped to std::weak_ptr for
Messenger in RpcRetrier.
The rationale for this change is to stop needles RPC retries when the
top-level object that created the messenger does not exist any longer.
Change-Id: I399eef35341c4c9e90d91b1e461302a985d8f283
---
M src/kudu/rpc/rpc.cc
M src/kudu/rpc/rpc.h
2 files changed, 9 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/16866/1
--
To view, visit http://gerrit.cloudera.org:8080/16866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I399eef35341c4c9e90d91b1e461302a985d8f283
Gerrit-Change-Number: 16866
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [rpc] weak ptr for Messenger in RpcRetrier
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16866 )
Change subject: [rpc] weak_ptr for Messenger in RpcRetrier
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16866/1/src/kudu/rpc/rpc.h
File src/kudu/rpc/rpc.h:
http://gerrit.cloudera.org:8080/#/c/16866/1/src/kudu/rpc/rpc.h@160
PS1, Line 160: std::shared_ptr<Messenger> messenger() const {
: return messenger_.lock();
: }
> Before this change as long the messenger_ ptr was not null during construct
Right: this method now can return an empty shared_ptr.
The crucial update on the caller's side is there: https://gerrit.cloudera.org/#/c/16866/1/src/kudu/rpc/rpc.cc@50
The current code doesn't check if the messenger passed to the constructor of RpcRetrier was empty anyways, so it might be empty from the very beginning. However, this patch adds more dynamics into the picture, and I guess it's worth updating the other call sites -- it's good call, indeed.
--
To view, visit http://gerrit.cloudera.org:8080/16866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I399eef35341c4c9e90d91b1e461302a985d8f283
Gerrit-Change-Number: 16866
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@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: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 16 Dec 2020 17:00:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] [rpc] weak ptr for Messenger in RpcRetrier
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16866 )
Change subject: [rpc] weak_ptr for Messenger in RpcRetrier
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16866/1/src/kudu/rpc/rpc.h
File src/kudu/rpc/rpc.h:
http://gerrit.cloudera.org:8080/#/c/16866/1/src/kudu/rpc/rpc.h@160
PS1, Line 160: std::shared_ptr<Messenger> messenger() const {
: return messenger_.lock();
: }
Before this change as long the messenger_ ptr was not null during construction of the object then messenger_ will remain not null.
With this change that switches to weak_ptr, a nullptr could be returned by this public function.
I don't see any callers being updated to check for possible nullptr. Why so?
--
To view, visit http://gerrit.cloudera.org:8080/16866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I399eef35341c4c9e90d91b1e461302a985d8f283
Gerrit-Change-Number: 16866
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@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: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 14 Dec 2020 18:31:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] [rpc] weak ptr for Messenger in RpcRetrier
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16866 )
Change subject: [rpc] weak_ptr for Messenger in RpcRetrier
......................................................................
Patch Set 1: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16866/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16866/1//COMMIT_MSG@12
PS1, Line 12: retries
For my own understanding, this would retry until it eventually timed out, right? Or would it fail some other way?
http://gerrit.cloudera.org:8080/#/c/16866/1//COMMIT_MSG@12
PS1, Line 12: needles
nit: needless
--
To view, visit http://gerrit.cloudera.org:8080/16866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I399eef35341c4c9e90d91b1e461302a985d8f283
Gerrit-Change-Number: 16866
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 12 Dec 2020 04:34:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] [rpc] weak ptr for Messenger in RpcRetrier
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/16866 )
Change subject: [rpc] weak_ptr for Messenger in RpcRetrier
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16866/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16866/1//COMMIT_MSG@12
PS1, Line 12: retries
> In general, yes. However, while doing so, it might try to access some fiel
Sorry, a bit confused here, is the dist test you linked for this patch? If so, it seems it introduced some flakiness?
--
To view, visit http://gerrit.cloudera.org:8080/16866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I399eef35341c4c9e90d91b1e461302a985d8f283
Gerrit-Change-Number: 16866
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@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: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 23 Dec 2020 19:50:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] [rpc] weak ptr for Messenger in RpcRetrier
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/16866 )
Change subject: [rpc] weak_ptr for Messenger in RpcRetrier
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16866/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16866/1//COMMIT_MSG@12
PS1, Line 12: retries
> The dist-test run at http://dist-test.cloudera.org/job?job_id=jenkins-slave
Got it, thanks a lot for the explanation!
--
To view, visit http://gerrit.cloudera.org:8080/16866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I399eef35341c4c9e90d91b1e461302a985d8f283
Gerrit-Change-Number: 16866
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@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: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 29 Dec 2020 20:22:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] [rpc] weak ptr for Messenger in RpcRetrier
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16866 )
Change subject: [rpc] weak_ptr for Messenger in RpcRetrier
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16866/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16866/1//COMMIT_MSG@12
PS1, Line 12: retries
> For my own understanding, this would retry until it eventually timed out, r
In general, yes. However, while doing so, it might try to access some fields from objects which are not longer there if RPC contains raw pointers and references to those. That happened in http://dist-test.cloudera.org/job?job_id=jenkins-slave.1607649291.2575691 before I moved txn_multi_manager to be a field member of the RPC itself.
--
To view, visit http://gerrit.cloudera.org:8080/16866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I399eef35341c4c9e90d91b1e461302a985d8f283
Gerrit-Change-Number: 16866
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 12 Dec 2020 04:54:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] [rpc] weak ptr for Messenger in RpcRetrier
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16866 )
Change subject: [rpc] weak_ptr for Messenger in RpcRetrier
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16866/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16866/1//COMMIT_MSG@12
PS1, Line 12: retries
> Sorry, a bit confused here, is the dist test you linked for this patch? If
The dist-test run at http://dist-test.cloudera.org/job?job_id=jenkins-slave.1607649291.2575691 was not produced by/with this patch. That one is referring to a run after e75aad09a8dbe295a11cc2042903b74faf72623e but before 49c922b5cc3e5d7ee885c6f355eec973a5334441. The latter fixed the issue I was referring to.
This patch is more about dropping the reference to Messenger once the top-level object that created it is out of scope, so there would be needless retries if some RPC are still in-flight. Basically, that's about freeing unneeded resources earlier.
--
To view, visit http://gerrit.cloudera.org:8080/16866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I399eef35341c4c9e90d91b1e461302a985d8f283
Gerrit-Change-Number: 16866
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@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: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 23 Dec 2020 20:04:21 +0000
Gerrit-HasComments: Yes