You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2017/11/16 04:46:19 UTC
[kudu-CR] Deflake tablet replica-test anchor and transaction count assertions
Hello Mike Percy,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/8564
to review the following change.
Change subject: Deflake tablet_replica-test anchor and transaction count assertions
......................................................................
Deflake tablet_replica-test anchor and transaction count assertions
Various assertions in this test failed to account for the fact that,
when a transaction finishes, its callback is triggered before it
unregisters itself from the transaction tracker. This means that, if we
wait for the callback and then immediately consult the transaction
tracker (or log anchor registry) we may still see the transaction,
causing the assertion to fail.
The fix here is to simply wrap such assertions with ASSERT_EVENTUALLY.
Prior to the test change, if I added a 10ms sleep in
TransactionDriver::Finalize() between callling the callback and
releasing the transaction from the tracker, I'd get consistent failures.
Now I can add such a sleep and the test still passes.
Change-Id: Icee241077558a16b8a5076ab0c059362e8e6f035
---
M src/kudu/tablet/tablet_replica-test.cc
1 file changed, 20 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/8564/1
--
To view, visit http://gerrit.cloudera.org:8080/8564
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icee241077558a16b8a5076ab0c059362e8e6f035
Gerrit-Change-Number: 8564
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] Deflake tablet replica-test anchor and transaction count assertions
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8564 )
Change subject: Deflake tablet_replica-test anchor and transaction count assertions
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8564
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icee241077558a16b8a5076ab0c059362e8e6f035
Gerrit-Change-Number: 8564
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 16 Nov 2017 21:13:24 +0000
Gerrit-HasComments: No
[kudu-CR] Deflake tablet replica-test anchor and transaction count assertions
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8564 )
Change subject: Deflake tablet_replica-test anchor and transaction count assertions
......................................................................
Patch Set 1: Code-Review+2
Nice detective work.
I think it would be a little neater to define a macro for ASSERT_NO_LOG_ANCHORS that implemented ASSER_EVENTUALLY([&] { AssertNoLogAnchors(); }); but this is fine.
--
To view, visit http://gerrit.cloudera.org:8080/8564
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icee241077558a16b8a5076ab0c059362e8e6f035
Gerrit-Change-Number: 8564
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 16 Nov 2017 22:04:55 +0000
Gerrit-HasComments: No
[kudu-CR] Deflake tablet replica-test anchor and transaction count assertions
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has removed a vote on this change.
Change subject: Deflake tablet_replica-test anchor and transaction count assertions
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/8564
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Icee241077558a16b8a5076ab0c059362e8e6f035
Gerrit-Change-Number: 8564
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] Deflake tablet replica-test anchor and transaction count assertions
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8564 )
Change subject: Deflake tablet_replica-test anchor and transaction count assertions
......................................................................
Deflake tablet_replica-test anchor and transaction count assertions
Various assertions in this test failed to account for the fact that,
when a transaction finishes, its callback is triggered before it
unregisters itself from the transaction tracker. This means that, if we
wait for the callback and then immediately consult the transaction
tracker (or log anchor registry) we may still see the transaction,
causing the assertion to fail.
The fix here is to simply wrap such assertions with ASSERT_EVENTUALLY.
Prior to the test change, if I added a 10ms sleep in
TransactionDriver::Finalize() between callling the callback and
releasing the transaction from the tracker, I'd get consistent failures.
Now I can add such a sleep and the test still passes.
Change-Id: Icee241077558a16b8a5076ab0c059362e8e6f035
Reviewed-on: http://gerrit.cloudera.org:8080/8564
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Mike Percy <mp...@apache.org>
Tested-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/tablet/tablet_replica-test.cc
1 file changed, 20 insertions(+), 11 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Mike Percy: Looks good to me, approved
Todd Lipcon: Verified
--
To view, visit http://gerrit.cloudera.org:8080/8564
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Icee241077558a16b8a5076ab0c059362e8e6f035
Gerrit-Change-Number: 8564
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Deflake tablet replica-test anchor and transaction count assertions
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8564 )
Change subject: Deflake tablet_replica-test anchor and transaction count assertions
......................................................................
Patch Set 1: Verified+1
Test failures are some jenkins issues. The earlier run passed IWYU and LINT, and the later one failed it due to workspaces having some cruft.
--
To view, visit http://gerrit.cloudera.org:8080/8564
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icee241077558a16b8a5076ab0c059362e8e6f035
Gerrit-Change-Number: 8564
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 16 Nov 2017 22:44:19 +0000
Gerrit-HasComments: No