You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org> on 2018/01/08 20:55:22 UTC
[kudu-CR] [mvcc] Fix watermark advancement in the absence of committed transactions
David Ribeiro Alves has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8964
Change subject: [mvcc] Fix watermark advancement in the absence of committed transactions
......................................................................
[mvcc] Fix watermark advancement in the absence of committed transactions
MvccSnapshot's 'none_committed_at_or_after_' (ncaof) watermark
indicates the point after which no transactions are committed.
This is used to cull redo deltas that cannot contain any
committed transactions under a snapshot, thus reducing work when
applying deltas.
By definition, this watermark is never supposed to be lower than
the 'all_committed_before_' watermark. If this invariant is broken
delta selection might be wrong by skipping whole delta files.
Normally the ncaof watermark is advanced when transactions are
marked as committed, in which case this is done correctly. However
there is problem when the 'all_committed_before_' watermark
is advanced and there are no committed or in flight transactions.
In this case the ncaof watermark might be left behind, manifesting
as incorrect delta application/skipping or even crashes.
This patch includes a fix and a very directed test that makes sure
it is correct. A follow up patch includes a new test in fuzz-itest
that would trigger a crash without the fix.
Change-Id: Icf94ea289204adf2eed797c3623cdea1b11b09db
---
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
3 files changed, 48 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/8964/1
--
To view, visit http://gerrit.cloudera.org:8080/8964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icf94ea289204adf2eed797c3623cdea1b11b09db
Gerrit-Change-Number: 8964
Gerrit-PatchSet: 1
Gerrit-Owner: David Ribeiro Alves <da...@gmail.com>
[kudu-CR] [mvcc] Fix watermark advancement in the absence of committed transactions
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8964 )
Change subject: [mvcc] Fix watermark advancement in the absence of committed transactions
......................................................................
Patch Set 2: Verified+1
Unrelated failure: "RaftConsensusITest.TestReplicaBehaviorViaRPC"
--
To view, visit http://gerrit.cloudera.org:8080/8964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf94ea289204adf2eed797c3623cdea1b11b09db
Gerrit-Change-Number: 8964
Gerrit-PatchSet: 2
Gerrit-Owner: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 09 Jan 2018 08:32:59 +0000
Gerrit-HasComments: No
[kudu-CR] [mvcc] Fix watermark advancement in the absence of committed transactions
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8964 )
Change subject: [mvcc] Fix watermark advancement in the absence of committed transactions
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf94ea289204adf2eed797c3623cdea1b11b09db
Gerrit-Change-Number: 8964
Gerrit-PatchSet: 2
Gerrit-Owner: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 09 Jan 2018 20:14:01 +0000
Gerrit-HasComments: No
[kudu-CR] [mvcc] Fix watermark advancement in the absence of committed transactions
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8964 )
Change subject: [mvcc] Fix watermark advancement in the absence of committed transactions
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8964/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8964/1//COMMIT_MSG@9
PS1, Line 9: ncaof
this abbreviation doesn't seem to match the first letters of those words :)
http://gerrit.cloudera.org:8080/#/c/8964/1/src/kudu/tablet/mvcc-test.cc
File src/kudu/tablet/mvcc-test.cc:
http://gerrit.cloudera.org:8080/#/c/8964/1/src/kudu/tablet/mvcc-test.cc@745
PS1, Line 745: // In this case we'll advance safe/clean time but not the 'none_committed_at_or_after_',
: // meaning this watermark remains lower than safe/clean time.
: /
i'm confused by this comment for this test... are you describing the test's status when it was failing?
--
To view, visit http://gerrit.cloudera.org:8080/8964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf94ea289204adf2eed797c3623cdea1b11b09db
Gerrit-Change-Number: 8964
Gerrit-PatchSet: 1
Gerrit-Owner: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 09 Jan 2018 00:03:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] [mvcc] Fix watermark advancement in the absence of committed transactions
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8964 )
Change subject: [mvcc] Fix watermark advancement in the absence of committed transactions
......................................................................
[mvcc] Fix watermark advancement in the absence of committed transactions
MvccSnapshot's 'none_committed_at_or_after_' (ncaoa) watermark
indicates the point after which no transactions are committed.
This is used to cull redo deltas that cannot contain any
committed transactions under a snapshot, thus reducing work when
applying deltas.
By definition, this watermark is never supposed to be lower than
the 'all_committed_before_' watermark. If this invariant is broken
delta selection might be wrong by skipping whole delta files.
Normally the ncaoa watermark is advanced when transactions are
marked as committed, in which case this is done correctly. However
there is problem when the 'all_committed_before_' watermark
is advanced and there are no committed or in flight transactions.
In this case the ncaoa watermark might be left behind, manifesting
as incorrect delta application/skipping or even crashes.
This patch includes a fix and a very directed test that makes sure
it is correct. A follow up patch includes a new test in fuzz-itest
that would trigger a crash without the fix.
Change-Id: Icf94ea289204adf2eed797c3623cdea1b11b09db
Reviewed-on: http://gerrit.cloudera.org:8080/8964
Tested-by: David Ribeiro Alves <da...@gmail.com>
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
3 files changed, 50 insertions(+), 1 deletion(-)
Approvals:
David Ribeiro Alves: Verified
Todd Lipcon: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/8964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Icf94ea289204adf2eed797c3623cdea1b11b09db
Gerrit-Change-Number: 8964
Gerrit-PatchSet: 3
Gerrit-Owner: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [mvcc] Fix watermark advancement in the absence of committed transactions
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has removed a vote on this change.
Change subject: [mvcc] Fix watermark advancement in the absence of committed transactions
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/8964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Icf94ea289204adf2eed797c3623cdea1b11b09db
Gerrit-Change-Number: 8964
Gerrit-PatchSet: 2
Gerrit-Owner: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [mvcc] Fix watermark advancement in the absence of committed transactions
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8964 )
Change subject: [mvcc] Fix watermark advancement in the absence of committed transactions
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8964/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8964/1//COMMIT_MSG@9
PS1, Line 9: ncaof
> this abbreviation doesn't seem to match the first letters of those words :)
Done
http://gerrit.cloudera.org:8080/#/c/8964/1/src/kudu/tablet/mvcc-test.cc
File src/kudu/tablet/mvcc-test.cc:
http://gerrit.cloudera.org:8080/#/c/8964/1/src/kudu/tablet/mvcc-test.cc@745
PS1, Line 745: // In this case we'll advance safe/clean time but not the 'none_committed_at_or_after_',
: // meaning this watermark remains lower than safe/clean time.
: /
> i'm confused by this comment for this test... are you describing the test's
right, my bad. fixed
--
To view, visit http://gerrit.cloudera.org:8080/8964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf94ea289204adf2eed797c3623cdea1b11b09db
Gerrit-Change-Number: 8964
Gerrit-PatchSet: 1
Gerrit-Owner: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 09 Jan 2018 01:32:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] [mvcc] Fix watermark advancement in the absence of committed transactions
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8964
to look at the new patch set (#2).
Change subject: [mvcc] Fix watermark advancement in the absence of committed transactions
......................................................................
[mvcc] Fix watermark advancement in the absence of committed transactions
MvccSnapshot's 'none_committed_at_or_after_' (ncaoa) watermark
indicates the point after which no transactions are committed.
This is used to cull redo deltas that cannot contain any
committed transactions under a snapshot, thus reducing work when
applying deltas.
By definition, this watermark is never supposed to be lower than
the 'all_committed_before_' watermark. If this invariant is broken
delta selection might be wrong by skipping whole delta files.
Normally the ncaoa watermark is advanced when transactions are
marked as committed, in which case this is done correctly. However
there is problem when the 'all_committed_before_' watermark
is advanced and there are no committed or in flight transactions.
In this case the ncaoa watermark might be left behind, manifesting
as incorrect delta application/skipping or even crashes.
This patch includes a fix and a very directed test that makes sure
it is correct. A follow up patch includes a new test in fuzz-itest
that would trigger a crash without the fix.
Change-Id: Icf94ea289204adf2eed797c3623cdea1b11b09db
---
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
3 files changed, 50 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/8964/2
--
To view, visit http://gerrit.cloudera.org:8080/8964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icf94ea289204adf2eed797c3623cdea1b11b09db
Gerrit-Change-Number: 8964
Gerrit-PatchSet: 2
Gerrit-Owner: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>