You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2020/02/23 08:07:11 UTC
[kudu-CR] mvcc: remove "safe time" terminology
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15277
Change subject: mvcc: remove "safe time" terminology
......................................................................
mvcc: remove "safe time" terminology
The MvccManager maintains an exclusive lower bound on the timestamps
allowed for assignment for new transactions. It refers to this as the
"safe time".
MVCC's usage of "safe time" is confusing because it is easily conflated
with the very similar "safe time" in the context of consensus: the
timestamp before which the leader promises not to assign a timestamp for
a new write. This is a distributed property of a tablet, managed by the
leader's consensus::TimeManager, whereas MVCC safe time is managed by
each replica.
This replaces MVCC 'safe_time_' and associated methods with the more
literal 'new_txn_timestamp_exc_lower_bound_', in hopes of making it
clearer 1) what "safe time" is by reducing the number of versions of the
term, and 2) how MVCC's "safe time" (now referred to as the "new
transaction timestamp lower bound") is used.
There are no functional changes in this patch.
Change-Id: I990478e20039a6ea541c1aa32a2a865f8e017b83
---
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tserver/tablet_server-test-base.cc
M src/kudu/tserver/tablet_service.cc
10 files changed, 139 insertions(+), 117 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/15277/1
--
To view, visit http://gerrit.cloudera.org:8080/15277
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I990478e20039a6ea541c1aa32a2a865f8e017b83
Gerrit-Change-Number: 15277
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
[kudu-CR] mvcc: remove "safe time" terminology
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: mvcc: remove "safe time" terminology
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/15277
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I990478e20039a6ea541c1aa32a2a865f8e017b83
Gerrit-Change-Number: 15277
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] mvcc: remove "safe time" terminology
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15277 )
Change subject: mvcc: remove "safe time" terminology
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/15277/1/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:
http://gerrit.cloudera.org:8080/#/c/15277/1/src/kudu/tablet/mvcc.h@194
PS1, Line 194: 'ts'
Should this be 'timestamp'? Below too.
http://gerrit.cloudera.org:8080/#/c/15277/1/src/kudu/tablet/mvcc.h@379
PS1, Line 379: i.e. a timestamp below which all transactions are either
: // committed or in-flight.
I didn't really understand this bit in the old comment too: isn't any transaction, by its very nature, either committed or in-flight? If we're being pedantic I guess there's a third state (aborted) but that doesn't seem relevant to this particular explanation.
http://gerrit.cloudera.org:8080/#/c/15277/1/src/kudu/tablet/mvcc.cc
File src/kudu/tablet/mvcc.cc:
http://gerrit.cloudera.org:8080/#/c/15277/1/src/kudu/tablet/mvcc.cc@199
PS1, Line 199: 'ts'
'timestamp'
--
To view, visit http://gerrit.cloudera.org:8080/15277
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I990478e20039a6ea541c1aa32a2a865f8e017b83
Gerrit-Change-Number: 15277
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sun, 23 Feb 2020 22:21:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] mvcc: remove "safe time" terminology
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15277 )
Change subject: mvcc: remove "safe time" terminology
......................................................................
mvcc: remove "safe time" terminology
The MvccManager maintains an exclusive lower bound on the timestamps
allowed for assignment for new transactions. It refers to this as the
"safe time".
MVCC's usage of "safe time" is confusing because it is easily conflated
with the very similar "safe time" in the context of consensus: the
timestamp before which the leader promises not to assign a timestamp for
a new write. This is a distributed property of a tablet, managed by the
leader's consensus::TimeManager, whereas MVCC safe time is managed by
each replica.
This replaces MVCC 'safe_time_' and associated methods with the more
literal 'new_txn_timestamp_exc_lower_bound_', in hopes of making it
clearer 1) what "safe time" is by reducing the number of versions of the
term, and 2) how MVCC's "safe time" (now referred to as the "new
transaction timestamp lower bound") is used.
There are no functional changes in this patch.
Change-Id: I990478e20039a6ea541c1aa32a2a865f8e017b83
Reviewed-on: http://gerrit.cloudera.org:8080/15277
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/integration-tests/timestamp_advancement-itest.cc
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tserver/tablet_server-test-base.cc
M src/kudu/tserver/tablet_service.cc
11 files changed, 141 insertions(+), 120 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Andrew Wong: Verified
--
To view, visit http://gerrit.cloudera.org:8080/15277
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I990478e20039a6ea541c1aa32a2a865f8e017b83
Gerrit-Change-Number: 15277
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] mvcc: remove "safe time" terminology
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15277
to look at the new patch set (#2).
Change subject: mvcc: remove "safe time" terminology
......................................................................
mvcc: remove "safe time" terminology
The MvccManager maintains an exclusive lower bound on the timestamps
allowed for assignment for new transactions. It refers to this as the
"safe time".
MVCC's usage of "safe time" is confusing because it is easily conflated
with the very similar "safe time" in the context of consensus: the
timestamp before which the leader promises not to assign a timestamp for
a new write. This is a distributed property of a tablet, managed by the
leader's consensus::TimeManager, whereas MVCC safe time is managed by
each replica.
This replaces MVCC 'safe_time_' and associated methods with the more
literal 'new_txn_timestamp_exc_lower_bound_', in hopes of making it
clearer 1) what "safe time" is by reducing the number of versions of the
term, and 2) how MVCC's "safe time" (now referred to as the "new
transaction timestamp lower bound") is used.
There are no functional changes in this patch.
Change-Id: I990478e20039a6ea541c1aa32a2a865f8e017b83
---
M src/kudu/integration-tests/timestamp_advancement-itest.cc
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/local_tablet_writer.h
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tserver/tablet_server-test-base.cc
M src/kudu/tserver/tablet_service.cc
11 files changed, 141 insertions(+), 120 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/15277/2
--
To view, visit http://gerrit.cloudera.org:8080/15277
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I990478e20039a6ea541c1aa32a2a865f8e017b83
Gerrit-Change-Number: 15277
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] mvcc: remove "safe time" terminology
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15277 )
Change subject: mvcc: remove "safe time" terminology
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15277
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I990478e20039a6ea541c1aa32a2a865f8e017b83
Gerrit-Change-Number: 15277
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sun, 23 Feb 2020 22:54:04 +0000
Gerrit-HasComments: No
[kudu-CR] mvcc: remove "safe time" terminology
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15277 )
Change subject: mvcc: remove "safe time" terminology
......................................................................
Patch Set 2: Verified+1
Unrelated flake of BadMultiMasterAuthzTokenTest.TestMasterElectionStorms
--
To view, visit http://gerrit.cloudera.org:8080/15277
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I990478e20039a6ea541c1aa32a2a865f8e017b83
Gerrit-Change-Number: 15277
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sun, 23 Feb 2020 23:21:22 +0000
Gerrit-HasComments: No
[kudu-CR] mvcc: remove "safe time" terminology
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15277 )
Change subject: mvcc: remove "safe time" terminology
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/15277/1/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:
http://gerrit.cloudera.org:8080/#/c/15277/1/src/kudu/tablet/mvcc.h@194
PS1, Line 194: 'tim
> Should this be 'timestamp'? Below too.
Done
http://gerrit.cloudera.org:8080/#/c/15277/1/src/kudu/tablet/mvcc.h@379
PS1, Line 379: timestamp at and below which no new transactions can be
: // initialized.
> I didn't really understand this bit in the old comment too: isn't any trans
Yeah that's fair. I think it was trying to say that any transaction below this timestamp must already have been in-flight or committed. I'll remove it since it doesn't add much, I don't think.
http://gerrit.cloudera.org:8080/#/c/15277/1/src/kudu/tablet/mvcc.cc
File src/kudu/tablet/mvcc.cc:
http://gerrit.cloudera.org:8080/#/c/15277/1/src/kudu/tablet/mvcc.cc@199
PS1, Line 199: 'tim
> 'timestamp'
Done
--
To view, visit http://gerrit.cloudera.org:8080/15277
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I990478e20039a6ea541c1aa32a2a865f8e017b83
Gerrit-Change-Number: 15277
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Sun, 23 Feb 2020 22:45:46 +0000
Gerrit-HasComments: Yes