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/02 00:36:22 UTC
[kudu-CR] KUDU-2088: Synchronizer may not go out of scope with outstanding references
Hello David Ribeiro Alves, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/7562
to review the following change.
Change subject: KUDU-2088: Synchronizer may not go out of scope with outstanding references
......................................................................
KUDU-2088: Synchronizer may not go out of scope with outstanding references
A common Kudu pattern is to declare a Synchronizer on the stack, create a
functor referencing it via AsStatusCallback, pass the functor into an
asynchronous function, wait until the functor is invoked (via Wait), then
allow the Synchronizer to go out of scope. This assumes that the async
function only invokes the StatusCallback once; a safe assumption in Kudu.
If using WaitFor instead of Wait, you must "fully wait" on the Synchronizer
before it goes out of scope. That is, WaitFor must be called repeatedly
until it no longer returns TimedOut, to ensure that whomever took the
reference has used it and will not use it again.
In this block of code, that invariant was violated. Namely, a failure in
SnoozeFailureDetector led to an early destruction of log_synchronizer,
causing a crash when the functor is invoked later. SnoozeFailureDetector
could fail if the node was elected leader while waiting on the Synchronizer.
Without the fix, raft_consensus-itest crashed in a particular way about one
or two percent of the time. With it, these crashes no longer manifest.
Change-Id: Icf9ce499616a872ad5ed4a27bd1e90e94fba5ff5
---
M src/kudu/consensus/raft_consensus.cc
1 file changed, 2 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/7562/1
--
To view, visit http://gerrit.cloudera.org:8080/7562
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icf9ce499616a872ad5ed4a27bd1e90e94fba5ff5
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: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2088: Synchronizer may not go out of scope with outstanding references
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-2088: Synchronizer may not go out of scope with outstanding references
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7562
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf9ce499616a872ad5ed4a27bd1e90e94fba5ff5
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-2088: Synchronizer may not go out of scope with outstanding references
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-2088: Synchronizer may not go out of scope with outstanding references
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7562/1//COMMIT_MSG
Commit Message:
PS1, Line 25: Without the fix, raft_consensus-itest crashed in a particular way about one
: or two percent of the time. With it, these crashes no longer manifest.
> I got too excited and pushed the commit early. I'm still looping raft_conse
Yep, it was borne out.
--
To view, visit http://gerrit.cloudera.org:8080/7562
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf9ce499616a872ad5ed4a27bd1e90e94fba5ff5
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2088: Synchronizer may not go out of scope with outstanding references
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.
Change subject: KUDU-2088: Synchronizer may not go out of scope with outstanding references
......................................................................
KUDU-2088: Synchronizer may not go out of scope with outstanding references
A common Kudu pattern is to declare a Synchronizer on the stack, create a
functor referencing it via AsStatusCallback, pass the functor into an
asynchronous function, wait until the functor is invoked (via Wait), then
allow the Synchronizer to go out of scope. This assumes that the async
function only invokes the StatusCallback once; a safe assumption in Kudu.
If using WaitFor instead of Wait, you must "fully wait" on the Synchronizer
before it goes out of scope. That is, WaitFor must be called repeatedly
until it no longer returns TimedOut, to ensure that whomever took the
reference has used it and will not use it again.
In this block of code, that invariant was violated. Namely, a failure in
SnoozeFailureDetector led to an early destruction of log_synchronizer,
causing a crash when the functor is invoked later. SnoozeFailureDetector
could fail if the node was elected leader while waiting on the Synchronizer.
Without the fix, raft_consensus-itest crashed in a particular way about one
or two percent of the time. With it, these crashes no longer manifest.
Change-Id: Icf9ce499616a872ad5ed4a27bd1e90e94fba5ff5
Reviewed-on: http://gerrit.cloudera.org:8080/7562
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Kudu Jenkins
---
M src/kudu/consensus/raft_consensus.cc
1 file changed, 2 insertions(+), 1 deletion(-)
Approvals:
Todd Lipcon: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/7562
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Icf9ce499616a872ad5ed4a27bd1e90e94fba5ff5
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2088: Synchronizer may not go out of scope with outstanding references
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-2088: Synchronizer may not go out of scope with outstanding references
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7562/1//COMMIT_MSG
Commit Message:
PS1, Line 25: Without the fix, raft_consensus-itest crashed in a particular way about one
: or two percent of the time. With it, these crashes no longer manifest.
I got too excited and pushed the commit early. I'm still looping raft_consensus-itest, but I'm confident that this result will be borne out.
--
To view, visit http://gerrit.cloudera.org:8080/7562
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf9ce499616a872ad5ed4a27bd1e90e94fba5ff5
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes