You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Hao Hao (Code Review)" <ge...@cloudera.org> on 2017/12/08 23:16:20 UTC
[kudu-CR] KUDU-1704: add BOUNDED READ scan mode
Hao Hao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8804
Change subject: KUDU-1704: add BOUNDED_READ scan mode
......................................................................
KUDU-1704: add BOUNDED_READ scan mode
This patch adds a new read mode BOUNDED_READ on tserver. In this mode,
the server will pick an arbitrarily snapshot in the past, subject to
the propagated timestamp bound, and perform a read. Moreover, the chosen
snapshot timestamp is not returned back to the client.
The major difference between READ_AT_SNAPSHOT scan without a timestamp
and BOUNDED_READ scan is the latter will choose the newest timestamp
within the staleness bound that allows execution of the reads without
blocking by the in-flight transactions. BOUNDED_READ mode is not
repeatable. However, it allows client local read-your-writes.
Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
---
M src/kudu/common/common.proto
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
4 files changed, 220 insertions(+), 37 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/1
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Mike Percy, Dan Burkert, David Ribeiro Alves, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8804
to look at the new patch set (#12).
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
KUDU-1704: add READ_YOUR_WRITES scan mode
This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode,
the server will pick a snapshot in the past, subject to the propagated
timestamp bound, and perform a read. Moreover, the chosen snapshot
timestamp is returned back to the client.
The major difference between READ_AT_SNAPSHOT scan without a timestamp
and READ_YOUR_WRITES scan is the latter will choose the newest timestamp
within the staleness bound that allows execution of the reads without
being blocked by the in-flight transactions. READ_YOUR_WRITES mode is not
repeatable. However, it allows client local
read-your-writes/read-your-reads.
Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing
Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
---
M src/kudu/common/common.proto
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
4 files changed, 234 insertions(+), 52 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/12
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 12
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 16:
(4 comments)
lgtm, just a nit and one last question
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2117
PS16, Line 2117: .
s/./,/
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2118
PS16, Line 2118: initiated
s/initiated with/default-constructed to/
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2118
PS16, Line 2118: Since
s/Since/since/
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2163
PS16, Line 2163: (propagation ts + 1) should not
is that guaranteed by something?
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 16
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 20 Feb 2018 20:47:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 18: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 18
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 21 Feb 2018 08:01:41 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8804
to look at the new patch set (#8).
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
KUDU-1704: add READ_YOUR_WRITES scan mode
This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode,
the server will pick a snapshot in the past, subject to the propagated
timestamp bound, and perform a read. Moreover, the chosen snapshot
timestamp is returned back to the client.
The major difference between READ_AT_SNAPSHOT scan without a timestamp
and READ_YOUR_WRITES scan is the latter will choose the newest timestamp
within the staleness bound that allows execution of the reads without
being blocked by the in-flight transactions. READ_OWN_WRITES mode is not
repeatable. However, it allows client local read-your-writes.
Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing
Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
---
M src/kudu/common/common.proto
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
4 files changed, 260 insertions(+), 52 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/8
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 8
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-1704: add BOUNDED READ scan mode
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add BOUNDED_READ scan mode
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/common/common.proto
File src/kudu/common/common.proto:
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/common/common.proto@221
PS1, Line 221: When BOUNDED_READ is specified, the server will pick an arbitrarily snapshot
: // in the past, subject to the propagated timestamp bound, and perform a read.
: // More specifically, the server will choose the newest timestamp within the
: // staleness bound that allows execution of the reads without blocking by the
: // in-flight transactions. If no propagated timestamp is provided the server
: // will take the timestamp which all transactions before it are committed.
Not a big fan of BOUNDED_READ (bounded by what?) but I've had trouble to come up with a good name in the past.
Some candidates:
READ_BOUNDED_STALE - Pros: formally descriptive. Cons: has STALE in the name :); Unfamiliar
READ_OWN_WRITES - Pros: familiar. Cons: People might interpret to mean ROW for all clients, while this is client local, but maybe OWN is good enough of a hint.
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/common/common.proto@228
PS1, Line 228: Boundedly stale reads are not repeatable: two stale reads, even if they
: // use the same staleness bound, can execute at different timestamps and thus
: // return inconsistent results. However, it allows read-your-writes for each
: // client, as the picked timestamp must be higher than the one of the last
: // write or read, known from the propagated timestamp.
Hum, if we're waiting for a clean timestamp then the read is repeatable, right? there is no reason why we couldn't scan again at that timestamp from this or from some other client
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/tserver/tablet_service.cc@2036
PS1, Line 2036: ReadMode read_mode = scan_pb.read_mode();
: if (read_mode != READ_AT_SNAPSHOT && read_mode != BOUNDED_READ) {
: return Status::NotSupported("Unsupported snapshot scan mode specified");
: }
nit: use switch case
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/tserver/tablet_service.cc@2041
PS1, Line 2041: // If the client sent a timestamp update our clock with it.
: if (scan_pb.has_propagated_timestamp()) {
: Timestamp propagated_timestamp(scan_pb.propagated_timestamp());
:
: // Update the clock so that we never generate snapshots lower that
: // 'propagated_timestamp'. If 'propagated_timestamp' is lower than
: // 'now' this call has no effect. If 'propagated_timestamp' is too much
: // into the future this will fail and we abort.
: RETURN_NOT_OK(server_->clock()->Update(propagated_timestamp));
: }
:
: Timestamp tmp_snap_timestamp;
: Tablet* tablet = tablet_replica->tablet();
: scoped_refptr<consensus::TimeManager> time_manager = tablet_replica->time_manager();
: tablet::MvccManager* mvcc_manager = tablet->mvcc_manager();
:
: if (read_mode == READ_AT_SNAPSHOT) {
: // For READ_AT_SNAPSHOT mode,
: // 1) if the client provided no snapshot timestamp we take the current
: // clock time as the snapshot timestamp.
: // 2) else we use the client provided one, but make sure it is not too
: // far in the future as to be invalid.
: if (!scan_pb.has_snap_timestamp()) {
: tmp_snap_timestamp = server_->clock()->Now();
: } else {
: tmp_snap_timestamp.FromUint64(scan_pb.snap_timestamp());
: RETURN_NOT_OK(ValidateTimeStamp(tmp_snap_timestamp));
: }
: } else {
: // For BOUNDED_READ mode,
: // 1) if the client provided no propagated timestamp we take the mvcc
: // 'clean' timestamp as the snapshot timestamp.
: // 2) else we use the MAX(propagated timestamp + 1, 'clean' timestamp)
: // and make sure it is not too far in the future as to be invalid.
: if (!scan_pb.has_propagated_timestamp()) {
: tmp_snap_timestamp = mvcc_manager->GetCleanTimestamp();
: } else {
: tmp_snap_timestamp.FromUint64(scan_pb.propagated_timestamp() + 1);
: tmp_snap_timestamp = tmp_snap_timestamp > mvcc_manager->GetCleanTimestamp() ?
: tmp_snap_timestamp : mvcc_manager->GetCleanTimestamp();
: RETURN_NOT_OK(ValidateTimeStamp(tmp_snap_timestamp));
: }
: }
:
: // Before we wait on anything check that the timestamp is after the AHM.
: // This is not the final check. We'll check this again after the iterators are open but
: // there is no point in waiting if we can't actually scan afterwards.
: RETURN_NOT_OK(VerifyNotAncientHistory(tablet_replica->tablet(),
: read_mode,
: tmp_snap_timestamp));
Can you move all of this to a new method? maybe PickOrValidateTimestamp()
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/tserver/tablet_service.cc@2136
PS1, Line 2136: // Do not return the picked snapshot timestamp for BOUNDED_READ mode.
why?
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 12 Dec 2017 02:04:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add BOUNDED READ scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add BOUNDED_READ scan mode
......................................................................
Patch Set 1:
The failed test ObjectIdGeneratorTest.TestCanoicalizeUuid seems to be flaky and should not be related to this change.
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Sat, 09 Dec 2017 00:04:36 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Mike Percy, Dan Burkert, David Ribeiro Alves, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8804
to look at the new patch set (#15).
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
KUDU-1704: add READ_YOUR_WRITES scan mode
This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode,
the server will pick a snapshot in the past, subject to the propagated
timestamp bound, and perform a read. Moreover, the chosen snapshot
timestamp is returned back to the client.
The major difference between READ_AT_SNAPSHOT scan without a timestamp
and READ_YOUR_WRITES scan is the latter will choose the newest timestamp
within the staleness bound that allows execution of the reads without
being blocked by the in-flight transactions if possible. READ_YOUR_WRITES
mode is not repeatable. However, it allows client local
read-your-writes/read-your-reads.
Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing
Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
---
M src/kudu/common/common.proto
M src/kudu/tserver/tablet_server-test-base.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
6 files changed, 321 insertions(+), 98 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/15
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 15
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] [WIP] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8804
to look at the new patch set (#5).
Change subject: [WIP] KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
[WIP] KUDU-1704: add READ_YOUR_WRITES scan mode
This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode,
the server will pick a snapshot in the past, subject to the propagated
timestamp bound, and perform a read. The chosen snapshot timestamp is
returned back to the client. READ_YOUR_WRITES mode is repeatable and
allows client local read-your-writes.
The major difference between READ_AT_SNAPSHOT scan without a timestamp
and READ_YOUR_WRITES scan is the latter will choose the newest timestamp
within the staleness bound that allows execution of the reads without
being blocked by the in-flight transactions.
Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
---
M src/kudu/common/common.proto
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
4 files changed, 276 insertions(+), 52 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/5
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-1704: add BOUNDED READ scan mode
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: KUDU-1704: add BOUNDED_READ scan mode
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1704: add READ OWN WRITES scan mode
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_OWN_WRITES scan mode
......................................................................
Patch Set 2:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/common/common.proto
File src/kudu/common/common.proto:
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/common/common.proto@222
PS2, Line 222: BOUNDED_READ
READ_OWN_WRITES ?
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1843
PS2, Line 1843: RpcController rpc;
nit: move this under the 'Send the call' scope.
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1855
PS2, Line 1855: s
nit: make it capital
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1917
PS2, Line 1917: ASSERT_LT(pre_scan_ts.ToUint64(), resp.propagated_timestamp());
Should the result scan contain any rows? Does it make sense to assert on that?
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1922
PS2, Line 1922: post
?
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_service.h
File src/kudu/tserver/tablet_service.h:
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_service.h@156
PS2, Line 156: Status PickAndVerifyTimeStamp(const NewScanRequestPB& scan_pb,
nit: add comments on what these two methods do.
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_service.cc@1619
PS2, Line 1619: BOUNDED_READ
nit: READ_OWN_WRITES?
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_service.cc@2153
PS2, Line 2153: BOUNDED_READ
nit: READ_OWN_WRITES ?
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_service.cc@2162
PS2, Line 2162: tmp_snap_timestamp = tmp_snap_timestamp > mvcc_manager->GetCleanTimestamp() ?
: tmp_snap_timestamp : mvcc_manager->GetCleanTimestamp();
nit: maybe, call GetCleanTimestamp() just once, storing the result in the temporary and use that for the comparison logic?
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 13 Dec 2017 17:19:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 16: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 16
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Sat, 17 Feb 2018 00:36:50 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Mike Percy, Dan Burkert, David Ribeiro Alves, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8804
to look at the new patch set (#18).
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
KUDU-1704: add READ_YOUR_WRITES scan mode
This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode,
the server will pick a snapshot in the past, subject to the propagated
timestamp bound, and perform a read. Moreover, the chosen snapshot
timestamp is returned back to the client.
The major difference between READ_AT_SNAPSHOT scan without a timestamp
and READ_YOUR_WRITES scan is the latter will choose the newest timestamp
within the staleness bound that allows execution of the reads without
being blocked by the in-flight transactions if possible. READ_YOUR_WRITES
mode is not repeatable. However, it allows client local
read-your-writes/read-your-reads.
Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing
Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
---
M src/kudu/common/common.proto
M src/kudu/tserver/tablet_server-test-base.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
6 files changed, 326 insertions(+), 98 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/18
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 18
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Mike Percy, Dan Burkert, David Ribeiro Alves, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8804
to look at the new patch set (#16).
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
KUDU-1704: add READ_YOUR_WRITES scan mode
This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode,
the server will pick a snapshot in the past, subject to the propagated
timestamp bound, and perform a read. Moreover, the chosen snapshot
timestamp is returned back to the client.
The major difference between READ_AT_SNAPSHOT scan without a timestamp
and READ_YOUR_WRITES scan is the latter will choose the newest timestamp
within the staleness bound that allows execution of the reads without
being blocked by the in-flight transactions if possible. READ_YOUR_WRITES
mode is not repeatable. However, it allows client local
read-your-writes/read-your-reads.
Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing
Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
---
M src/kudu/common/common.proto
M src/kudu/tserver/tablet_server-test-base.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
6 files changed, 321 insertions(+), 98 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/16
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 16
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 14:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8804/13/src/kudu/tserver/tablet_server-test-base.cc
File src/kudu/tserver/tablet_server-test-base.cc:
http://gerrit.cloudera.org:8080/#/c/8804/13/src/kudu/tserver/tablet_server-test-base.cc@423
PS13, Line 423: ScanRequestPB req;
> warning: parameter 'read_mode' is unused [misc-unused-parameters]
Done
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 14
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 16 Feb 2018 00:00:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ OWN WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_OWN_WRITES scan mode
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/common/common.proto
File src/kudu/common/common.proto:
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/common/common.proto@228
PS1, Line 228: transactions before it are committed.
: //
: // Reads in this mode are not repeatable: two stale reads, even if they
: // provide the same propagated timestamp bound, can execute at different
: // timestamps and thus return inconsistent results. Ho
> We talked offline a bit about this and the problem is mostly one of nomencl
I would lean to the second option, that stop calling READ_AT_SNAPSHOT without a timestamp repeatable reads. Rename it to READ_CONSISTENT looks good to me but we may also need to concern about backward compatibility. My main concern of calling it repeatable read is the users may get confused when the same bound is given, different results are returned (because different timestamp is chosen).
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 15 Dec 2017 01:11:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 10:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto
File src/kudu/common/common.proto:
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@231
PS10, Line 231: stale
The 'stale' terminology hasn't been introduced here, so I think it would be best to avoid it. Perhaps 'two READ_YOUR_WRITES scans, ...'
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@233
PS10, Line 233: inconsistent
I think it would be better to say 'different results' here in order to avoid confusion over the formal consistency guarantees.
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@237
PS10, Line 237: READ_YOUR_WRITES = 3;
It may be good to add docs about whether a snapshot timestamp and propagated timestamp are returned, and what the client should do with them.
I'm surprised to see in the latest revision that READ_YOUR_WRITES doesn't return a snapshot timestamp, why is that? And is the returned propagated timestamp set to the snapshot timestamp of the scan?
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 02 Feb 2018 17:17:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ OWN WRITES scan mode
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_OWN_WRITES scan mode
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/common/common.proto
File src/kudu/common/common.proto:
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/common/common.proto@228
PS1, Line 228: Boundedly stale reads are not repeatable: two stale reads, even if they
: // use the same staleness bound, can execute at different timestamps and thus
: // return inconsistent results. However, it allows read-your-writes for each
: // client, as the picked timestamp must be higher than the one of the last
: // write or read, known from the propagated timestamp.
> Right, but I think the point is each read the server may pick different tim
We talked offline a bit about this and the problem is mostly one of nomenclature. On the one side Spanner originally did make the distinction between this mode (bounded staleness read) and repeatable reads, but spanner makes the client choose a timestamp and even hints that the client should choose one in the recent past. In addition to this we also have READ_AT_SNAPSHOT without a timestamp, that we also call repeatable read (and that would be a repeatable read with a dynamic timestamp, like this one). So either we also call this repeatable read or we stop calling the other one so, maybe even change the name (to READ_CONSISTENT or whatever).
Wdyt?
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 15 Dec 2017 00:26:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 16:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_server-test.cc@a1726
PS16, Line 1726:
> What happened to this test? Maybe I'm missing something, but it seems this
Since ScannerOpenWhenServerShutsDownParamTest should also cover the scenario of this test case. I removed it as it is duplicated.
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h
File src/kudu/tserver/tablet_service.h:
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h@157
PS16, Line 157: TimeStamp
> nit: in the code around, we have Timestamp as a type; maybe, name it consis
Done
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h@161
PS16, Line 161: PickAndVerifyTimeStamp
> nit: ditto, PickAndVerifyTimestamp(...) ?
Done
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h@162
PS16, Line 162: tablet::TabletReplica*
> Could it be const tablet::TabletReplica& ?
Done
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@1757
PS16, Line 1757: case READ_YOUR_WRITES:
> nit: per coding standard, add // fallthrough
Done
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2042
PS16, Line 2042: case READ_AT_SNAPSHOT:
> nit: per coding standard, add // fallthrough
Done
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2117
PS16, Line 2117: .
> s/./,/
Done
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2118
PS16, Line 2118: initiated
> s/initiated with/default-constructed to/
Done
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2118
PS16, Line 2118: Since
> s/Since/since/
Done
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2163
PS16, Line 2163: (propagation ts + 1) should not
> is that guaranteed by something?
As we update the clock based on propagated timestamp at L2140, if (propagation ts + 1) is a timestamp too far in the future, which essentially means (propagation TS + 1 > propagation TS + FLAGS_max_clock_sync_error_usec).
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 16
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 20 Feb 2018 23:50:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 14:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.h
File src/kudu/tserver/tablet_service.h:
http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.h@156
PS14, Line 156: // as such timestamp is invalid.
> nit: "such a"
Done
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2114
PS12, Line 2114: }
:
> Good question/point. I wrote that and it was cryptic to me too. I now reali
I see, thanks David for the explanation.
http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.cc@2046
PS14, Line 2046: return Status::NotSupported("Unsupported snapshot scan mode specified");
> nit: I think this can FATAL out. it's us (not the user) that call this and
Done
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 14
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 16 Feb 2018 23:26:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 10:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto
File src/kudu/common/common.proto:
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@222
PS10, Line 222: snapshot
nit: snapshot timestamp
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@228
PS10, Line 228: take
nit: choose
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@229
PS10, Line 229: which
nit: such that
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1922
PS10, Line 1922: TestScanYourWrites
We decided we also wanted scan your reads right? is that covered?
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1944
PS10, Line 1944: // Send the call
I know this is likely copy/paste but remove (adds nothing)
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1953
PS10, Line 1953: // Make sure that there is no snapshot timestamp sent back.
: ASSERT_TRUE(!resp.has_snap_timestamp());
I'm still unsure why we wouldn't the response to include the timestamp. I agree that the scan itself (seen as a collection os scans to multiple tablets) can't be described by a single timestamp, but having the timestamp in the response would allows to have fault-tolerant scans, right?
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1989
PS10, Line 1989: // Send the call
same
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.h
File src/kudu/tserver/tablet_service.h:
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.h@158
PS10, Line 158: on the read mode,
on the read mode?
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.cc@2107
PS10, Line 2107: s.IsNotSupported()
nit add a predict false
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.cc@2161
PS10, Line 2161: 0
use kMinTimestamp or something? I understand you can't use kInvalidTimestamp (it's big) but don't want to accidentally set time to 0 (been there done that :))
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.cc@2163
PS10, Line 2163: tmp_snap_timestamp > clean_timestamp ?
: tmp_snap_timestamp : clean_timestamp;
std:max ?
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 02 Feb 2018 22:33:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 16:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_server-test.cc@a1726
PS16, Line 1726:
What happened to this test? Maybe I'm missing something, but it seems this test should stay relevant regardless of the new scan mode.
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h
File src/kudu/tserver/tablet_service.h:
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h@157
PS16, Line 157: TimeStamp
nit: in the code around, we have Timestamp as a type; maybe, name it consistently with that so it's ValidateTimestamp(...) ?
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h@161
PS16, Line 161: PickAndVerifyTimeStamp
nit: ditto, PickAndVerifyTimestamp(...) ?
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h@162
PS16, Line 162: tablet::TabletReplica*
Could it be const tablet::TabletReplica& ?
Or, maybe, Tablet* ?
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@1757
PS16, Line 1757: case READ_YOUR_WRITES:
nit: per coding standard, add // fallthrough
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2042
PS16, Line 2042: case READ_AT_SNAPSHOT:
nit: per coding standard, add // fallthrough
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 16
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Sat, 17 Feb 2018 01:44:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 17:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2163
PS16, Line 2163: (propagation ts + 1) should not
> Maybe we should just add that explanation to the docs?
Thanks for the explanation. Let's add that explanation in the comment here, maybe something like:
// fall into that category because calling clock->Update(propagated_timestamp) verifies that propagated_timestamp + FLAGS_max_clock_sync_error_usec is not too far into the future.
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 17
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 21 Feb 2018 01:36:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add BOUNDED READ scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add BOUNDED_READ scan mode
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/common/common.proto
File src/kudu/common/common.proto:
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/common/common.proto@228
PS1, Line 228: Boundedly stale reads are not repeatable: two stale reads, even if they
: // use the same staleness bound, can execute at different timestamps and thus
: // return inconsistent results. However, it allows read-your-writes for each
: // client, as the picked timestamp must be higher than the one of the last
: // write or read, known from the propagated timestamp.
> Hum, if we're waiting for a clean timestamp then the read is repeatable, ri
Right, but I think the point is each read the server may pick different timestamps. Even given the same propagated time stamp, the 'clean' timestamp may not be the same at different time. Or maybe I missed something?
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/tserver/tablet_service.cc@2136
PS1, Line 2136: // Do not return the picked snapshot timestamp for BOUNDED_READ mode.
> why?
I agree with your comment on the Jira "Moreover each server should be free to choose a timestamp such as this, as long as it respects the aforementioned condition, which is why we shouldn't reuse timestamps across servers in this mode and shouldn't return the chosen snapshot timestamp back to the client." So here I did not return the timestamp. :) Does this still sound reasonable?
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 12 Dec 2017 21:43:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 12:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8804/12//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8804/12//COMMIT_MSG@15
PS12, Line 15: choose the newest timestamp
: within the staleness bound that allows execution of the reads without
: being blocked by the in-flight transactions
might be worth adding "if possible"
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1924
PS12, Line 1924: TestScanYourWrites
It worries me a bit that most tests of this functionality overwhelmingly tests the "happy" case. A good test of that is: if you changed the mode of all tests here to READ_LATEST would it fail? if not that's ok as long as we have some test somewhere that would, however I didn't see such a tests even after the client implementations.
Jepsen tests are good to cover more possibilities, but some deterministic tests just for RYW should exist.
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 12
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 09 Feb 2018 20:09:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 12:
(20 comments)
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto
File src/kudu/common/common.proto:
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto@236
PS12, Line 236: subject to the propagated timestamp bound
This wording is pretty hard to a casual reader (or even me) to understand.
I think it would be more clear to say something like: Each tablet server will choose a timestamp to use for a server-local snapshot scan subject to the following criteria: (1) It will be higher than the propagated timestamp, (2) It will attempt to minimize latency caused by waiting for outstanding write transactions to complete.
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto@237
PS12, Line 237: newest timestamp within
latest timestamp higher than
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto@242
PS12, Line 242: the timestamp
a timestamp
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto@244
PS12, Line 244: should use it
Is this something that end-users have to worry about, or can we word this so that it sounds like Kudu will take care of it? If the latter, how about
"The Kudu client library will use it as the propagated timestamp for subsequent reads" or something along those lines? However I'm not sure why we are making the distinction here between a normally propagated timestamp and this different thing.
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto@245
PS12, Line 245: wait
nit: waiting
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
PS12:
We are missing a C++ client test in this patch. Is that excluded on purpose?
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1924
PS12, Line 1924: TestScanYourWrites
> It worries me a bit that most tests of this functionality overwhelmingly te
I agree that a deterministic test would be nice and I think it should be possible using two clients.
It would also be nice to have a non-deterministic test where you have two clients in RYW mode reading and scanning. Loop them concurrently in separate threads at least a certain number of rounds or amount of time, as well as until both of the following conditions hold, while also triggering leader elections to make it easier to hit anomalies:
1. neither has ever violated RYW locally
2. we have observed anomalies where if we had been using a strict serializable approach then certain writes would have appeared, but they don't because of the hidden channel (the other client).
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1924
PS12, Line 1924: TEST_F(TabletServerTest, TestScanYourWrites) {
nit: add a comment at the top of this test describing what the test does
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1927
PS12, Line 1927: e
nit: missing period
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1958
PS12, Line 1958: TEST_F(TabletServerTest, TestScanYourWrites_WithoutPropagatedTimestamp) {
nit: add test comment
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1960
PS12, Line 1960: perform a write
nit: use capitalization and punctuation for code comments per https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_Grammar
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1972
PS12, Line 1972: perform a write
punctuation
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.h
File src/kudu/tserver/tablet_service.h:
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.h@155
PS12, Line 155: // it exceeds the maximum allowed clock synchronization error time.
Can we add a note about the context, i.e. why this matters?
nit: Also perhaps rename to ValidateSnapshotTimestamp() ? Usually Stamp in Timestamp is not capitalized in the Kudu code, and this seems specific to snapshot timestamps.
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.h@158
PS12, Line 158: if
s/if/that/
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2113
PS12, Line 2113:
add: DCHECK(s.ok()) ?
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2114
PS12, Line 2114: // Note: if 'max_allowed_ts' is not obtained from clock_->GetGlobalLatest() it's guaranteed
: // to be higher than 'tmp_snap_timestamp'.
I'm having trouble following this:
1. What does "not obtained" mean? Is it like GetGlobalLatest() returned Status::Corrruption or something like that and so then we assume max_allowed_ts is still default-constructed?
2. Why is it guaranteed to by higher than 'tmp_snap_timestamp'? Can we explain what guarantees that and why?
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2133
PS12, Line 2133: that
than
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2135
PS12, Line 2135: much
far
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2164
PS12, Line 2164: RETURN_NOT_OK(ValidateTimeStamp(tmp_snap_timestamp));
Hmm, we already updated the clock on line 2137 so AFAICT the only way this could fail is if the propagated timestamp +1 fails this test or the clean timestamp fails this test. Is that the purpose of this? Should we be trying to check this before we update the clock? I'm actually not sure what the right answer here is but intuitively it seems we should try to perform as much validation as possible before making changes to the system.
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2170
PS12, Line 2170: VerifyNotAncientHistory
same here; is there some way to check this before updating the clock, or we don't really care one way or the other?
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 12
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 13 Feb 2018 23:55:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ OWN WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_OWN_WRITES scan mode
......................................................................
Patch Set 2:
(20 comments)
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/common/common.proto
File src/kudu/common/common.proto:
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/common/common.proto@222
PS2, Line 222: BOUNDED_READ
> READ_OWN_WRITES ?
Done
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/common/common.proto@222
PS2, Line 222: pick an arbitrarily
> nit: "arbitrarily pick" or "pick an arbitrary"
Right, the picked snapshot is subject to the criteria you mentioned. Maybe 'arbitrary' is not a good word here. I will remove it. The intention was to specify the snapshot is not something specifically controlled by user, even the rule to chose one is specific.
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/common/common.proto@225
PS2, Line 225: staleness bound that allows execution of the reads
> This "staleness bound" isn't mentioned anywhere else, so I'm not sure exact
Done
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/common/common.proto@229
PS2, Line 229: Boundedly stale reads
> nit: maybe "Reads in this mode"?
Done
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@a1616
PS2, Line 1616:
:
:
> nit: various spurious deletions of newlines
I thought only one line is needed between tests?
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1835
PS2, Line 1835: TestBoundedScan
> nit: this should be renamed (same with other tests)
Done
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1843
PS2, Line 1843: RpcController rpc;
> nit: move this under the 'Send the call' scope.
Done
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1855
PS2, Line 1855: s
> nit: make it capital
Done
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1866
PS2, Line 1866: the no snapshot
> nit: typo, same in other tests.
Done
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1869
PS2, Line 1869: resp.has_propagated_timestamp()
> nit: maybe pull into a local variable?
I assume you mean 'resp.propagated_timestamp()'?
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1878
PS2, Line 1878: num_rows
> micro-nit: kNumRows?
Done
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1899
PS2, Line 1899: req.set_batch_size_bytes(0); // so it won't return data right away
> Is this needed here? I thought we only needed it to postpone draining the s
Done
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1917
PS2, Line 1917: ASSERT_LT(pre_scan_ts.ToUint64(), resp.propagated_timestamp());
> Should the result scan contain any rows? Does it make sense to assert on t
I do not think it makes sense to assert on that, since mvcc clean timestamp is hard to predefined.
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1922
PS2, Line 1922: post
> ?
Done
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1968
PS2, Line 1968: ASSERT_NO_FATAL_FAILURE
> nit: I think you can use the shorter "NO_FATALS" here
Done
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_service.h
File src/kudu/tserver/tablet_service.h:
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_service.h@156
PS2, Line 156: Status PickAndVerifyTimeStamp(const NewScanRequestPB& scan_pb,
> nit: add comments on what these two methods do.
Done
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_service.cc@1619
PS2, Line 1619: BOUNDED_READ
> nit: READ_OWN_WRITES?
Done
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_service.cc@2096
PS2, Line 2096: BOUNDED_READ
> nit: READ_OWN_WRITES
Done
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_service.cc@2153
PS2, Line 2153: BOUNDED_READ
> nit: READ_OWN_WRITES ?
Done
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_service.cc@2162
PS2, Line 2162: tmp_snap_timestamp = tmp_snap_timestamp > mvcc_manager->GetCleanTimestamp() ?
: tmp_snap_timestamp : mvcc_manager->GetCleanTimestamp();
> nit: maybe, call GetCleanTimestamp() just once, storing the result in the t
Done
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 14 Dec 2017 07:33:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 14:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.h
File src/kudu/tserver/tablet_service.h:
http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.h@156
PS14, Line 156: // as such timestamp is invalid.
nit: "such a"
http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.cc@2046
PS14, Line 2046: return Status::NotSupported("Unsupported snapshot scan mode specified");
nit: I think this can FATAL out. it's us (not the user) that call this and we should only call it for "snapshotty" scans
http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.cc@2166
PS14, Line 2166: RETURN_NOT_OK(ValidateTimeStamp(tmp_snap_timestamp));
What is this really checking. We need to validate the timestamp for READ_AT_SNAPSHOT as the user can send a timestamp that is arbitrarily in the future, but here it can only be either "clean_timestamp" (which is valid) or propagated timestamp, which is necessarily valid since we've udpated the clock with it in line 2139.
In other words do you think that there is a test case you can write that would fail this check?
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 14
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 16 Feb 2018 20:08:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ OWN WRITES scan mode
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_OWN_WRITES scan mode
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/common/common.proto
File src/kudu/common/common.proto:
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/common/common.proto@228
PS1, Line 228: Boundedly stale reads are not repeatable: two stale reads, even if they
: // use the same staleness bound, can execute at different timestamps and thus
: // return inconsistent results. However, it allows read-your-writes for each
: // client, as the picked timestamp must be higher than the one of the last
: // write or read, known from the propagated timestamp.
> I would lean to the second option, that stop calling READ_AT_SNAPSHOT witho
makes sense. shall we preface this patch with that change then.
we can make the change in a backward compatible manner by having the old mode be transformed to the new mode, while issuing a warning (logged only once).
To be clear, let's add a new mode, READ_CONSISTENT, that is equivalent to the current READ_AT_SNAPSHOT with no timestamp and reword whenever we refer to repeatable read to mean READ_AT_SNAPSHOT.
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 15 Dec 2017 21:40:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2114
PS12, Line 2114: // Note: if 'max_allowed_ts' is not obtained from clock_->GetGlobalLatest() it's guaranteed
: // to be higher than 'tmp_snap_timestamp'.
> Good question. TBH, this is refactored from previous code, so I am not sure
Good question/point. I wrote that and it was cryptic to me too. I now realize that I'm just referring to the fact that if we don't set a timestamp at all, it get's set to MAX_LONG - 1 (kInvalidTimestamp). The point is to put reader at ease that even if we don't get a global latest ts from the clock, it's still safe to compare the snap timestamp below.
Thanks for fixing this Hao
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 12
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 16 Feb 2018 22:50:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] [WIP] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: [WIP] KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/common/common.proto
File src/kudu/common/common.proto:
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/common/common.proto@228
PS1, Line 228: transactions). If no propagated timestamp is provided the server will take
: // the timestamp which all transactions before it are committed.
: //
: // READ_YOUR_WRITES is repeatable, i.e. all future reads at the same timestamp
: // will yield the same rows. It also allows read-your-
> Yes, will do, thanks!
After more offline discussions and doing more research on the definition of RR (The expected behavior is that effects of a committed transaction cannot become visible in the middle of execution of a transaction), I think in the context of single row write transaction, READ_AT_SNAPSHOT without a timestamp is repeatable as well. Therefore, I think the above patch is no longer needed.
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 5
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 12 Jan 2018 03:54:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 18:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2163
PS16, Line 2163:
> Thanks for the explanation. Let's add that explanation in the comment here,
Done
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2163
PS16, Line 2163:
> Maybe we should just add that explanation to the docs?
Updated.
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 18
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 21 Feb 2018 05:23:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Mike Percy, Dan Burkert, David Ribeiro Alves, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8804
to look at the new patch set (#11).
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
KUDU-1704: add READ_YOUR_WRITES scan mode
This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode,
the server will pick a snapshot in the past, subject to the propagated
timestamp bound, and perform a read. Moreover, the chosen snapshot
timestamp is returned back to the client.
The major difference between READ_AT_SNAPSHOT scan without a timestamp
and READ_YOUR_WRITES scan is the latter will choose the newest timestamp
within the staleness bound that allows execution of the reads without
being blocked by the in-flight transactions. READ_YOUR_WRITES mode is not
repeatable. However, it allows client local
read-your-writes/read-your-reads.
Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing
Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
---
M src/kudu/common/common.proto
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
4 files changed, 234 insertions(+), 52 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/11
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 11
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-1704: add READ OWN WRITES scan mode
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_OWN_WRITES scan mode
......................................................................
Patch Set 2:
(11 comments)
mostly nits
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/common/common.proto
File src/kudu/common/common.proto:
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/common/common.proto@222
PS2, Line 222: pick an arbitrarily
nit: "arbitrarily pick" or "pick an arbitrary"
BTW, what's arbitrary about the snapshot? It seems like it's not arbitrary since it's subject to a specific bound. Or is it selected randomly within that bound?
Based on the rest of the comment, it seems like the chosen snapshot is pretty specific: "the newest timestamp within the staleness bound"
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/common/common.proto@225
PS2, Line 225: staleness bound that allows execution of the reads
This "staleness bound" isn't mentioned anywhere else, so I'm not sure exactly what it's referring to. Mind rewording this a bit? Same in other places where "staleness bound" is used.
Maybe: "the newest timestamp that would allow reads at the provided propagated timestamp..." or something?
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/common/common.proto@229
PS2, Line 229: Boundedly stale reads
nit: maybe "Reads in this mode"?
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@a1616
PS2, Line 1616:
:
:
nit: various spurious deletions of newlines
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1835
PS2, Line 1835: TestBoundedScan
nit: this should be renamed (same with other tests)
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1866
PS2, Line 1866: the no snapshot
nit: typo, same in other tests.
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1869
PS2, Line 1869: resp.has_propagated_timestamp()
nit: maybe pull into a local variable?
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1878
PS2, Line 1878: num_rows
micro-nit: kNumRows?
Or maybe use one row instead of 100? That way we only need to check one result, unless checking multiple is important.
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1899
PS2, Line 1899: req.set_batch_size_bytes(0); // so it won't return data right away
Is this needed here? I thought we only needed it to postpone draining the scanner or something (not sure).
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_server-test.cc@1968
PS2, Line 1968: ASSERT_NO_FATAL_FAILURE
nit: I think you can use the shorter "NO_FATALS" here
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/8804/2/src/kudu/tserver/tablet_service.cc@2096
PS2, Line 2096: BOUNDED_READ
nit: READ_OWN_WRITES
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 14 Dec 2017 03:35:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] [WIP] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8804
to look at the new patch set (#6).
Change subject: [WIP] KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
[WIP] KUDU-1704: add READ_YOUR_WRITES scan mode
This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode,
the server will pick a snapshot in the past, subject to the propagated
timestamp bound, and perform a read. The chosen snapshot timestamp is
returned back to the client. READ_YOUR_WRITES mode is repeatable and
allows client local read-your-writes.
The major difference between READ_AT_SNAPSHOT scan without a timestamp
and READ_YOUR_WRITES scan is the latter will choose the newest timestamp
within the staleness bound that allows execution of the reads without
being blocked by the in-flight transactions.
Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
---
M src/kudu/common/common.proto
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
4 files changed, 269 insertions(+), 52 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/6
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 10:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/common/common.proto
File src/kudu/common/common.proto:
http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/common/common.proto@227
PS7, Line 227: som
> might be more clear to replace 'any' with 'some' here.
Done
http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/common/common.proto@227
PS7, Line 227:
> in
Done
http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/common/common.proto@231
PS7, Line 231: Reads in this mode are not repeatable: two stale reads, even if they
: // provide the same propaga
> I think it's worth going into a little more detail - READ_YOUR_WRITES is by
Hmm, I actually updated this in the latest patch (patch 9). Sorry for any confusion.
http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/common/common.proto@233
PS7, Line 233: mps an
> s/picked/chosen
Done
http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/tserver/tablet_service.h
File src/kudu/tserver/tablet_service.h:
http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/tserver/tablet_service.h@154
PS7, Line 154: so far in the future that
: // it exce
> 'not so far in the future that it exceeds'
Done
http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/tserver/tablet_service.cc@1625
PS7, Line 1625: t
> not your fault, but this looks like an extraneous quote.
Done
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 01 Feb 2018 22:45:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ OWN WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello David Ribeiro Alves, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8804
to look at the new patch set (#2).
Change subject: KUDU-1704: add READ_OWN_WRITES scan mode
......................................................................
KUDU-1704: add READ_OWN_WRITES scan mode
This patch adds a new read mode READ_OWN_WRITES on tserver. In this mode,
the server will pick an arbitrarily snapshot in the past, subject to
the propagated timestamp bound, and perform a read. Moreover, the chosen
snapshot timestamp is not returned back to the client.
The major difference between READ_AT_SNAPSHOT scan without a timestamp
and READ_OWN_WRITES scan is the latter will choose the newest timestamp
within the staleness bound that allows execution of the reads without
blocking by the in-flight transactions. READ_OWN_WRITES mode is not
repeatable. However, it allows client local read-your-writes.
Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
---
M src/kudu/common/common.proto
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
4 files changed, 255 insertions(+), 52 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/2
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 18: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_server-test.cc@a1726
PS16, Line 1726:
> Since ScannerOpenWhenServerShutsDownParamTest should also cover the scenari
Ah, I see. Thanks for the explanation.
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 18
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 21 Feb 2018 19:08:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Mike Percy, Dan Burkert, David Ribeiro Alves, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8804
to look at the new patch set (#17).
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
KUDU-1704: add READ_YOUR_WRITES scan mode
This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode,
the server will pick a snapshot in the past, subject to the propagated
timestamp bound, and perform a read. Moreover, the chosen snapshot
timestamp is returned back to the client.
The major difference between READ_AT_SNAPSHOT scan without a timestamp
and READ_YOUR_WRITES scan is the latter will choose the newest timestamp
within the staleness bound that allows execution of the reads without
being blocked by the in-flight transactions if possible. READ_YOUR_WRITES
mode is not repeatable. However, it allows client local
read-your-writes/read-your-reads.
Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing
Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
---
M src/kudu/common/common.proto
M src/kudu/tserver/tablet_server-test-base.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
6 files changed, 322 insertions(+), 98 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/17
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 17
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Mike Percy, Dan Burkert, David Ribeiro Alves, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8804
to look at the new patch set (#10).
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
KUDU-1704: add READ_YOUR_WRITES scan mode
This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode,
the server will pick a snapshot in the past, subject to the propagated
timestamp bound, and perform a read. Moreover, the chosen snapshot
timestamp is returned back to the client.
The major difference between READ_AT_SNAPSHOT scan without a timestamp
and READ_YOUR_WRITES scan is the latter will choose the newest timestamp
within the staleness bound that allows execution of the reads without
being blocked by the in-flight transactions. READ_OWN_WRITES mode is not
repeatable. However, it allows client local read-your-writes.
Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing
Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
---
M src/kudu/common/common.proto
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
4 files changed, 260 insertions(+), 52 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/10
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 7:
(6 comments)
Just a few nits on docs, but otherwise LGTM.
http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/common/common.proto
File src/kudu/common/common.proto:
http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/common/common.proto@227
PS7, Line 227: n
in
http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/common/common.proto@227
PS7, Line 227: any
might be more clear to replace 'any' with 'some' here.
http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/common/common.proto@231
PS7, Line 231: READ_YOUR_WRITES is repeatable, i.e. all future reads at the same timestamp
: // will yield the same rows
I think it's worth going into a little more detail - READ_YOUR_WRITES is by itself not repeatable, but a READ_YOUR_WRITES followed by a READ_AT_SNAPSHOT with the same timestamp chosen is repeatable.
http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/common/common.proto@233
PS7, Line 233: picked
s/picked/chosen
http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/tserver/tablet_service.h
File src/kudu/tserver/tablet_service.h:
http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/tserver/tablet_service.h@154
PS7, Line 154: too far in the future that
: // exceeds
'not so far in the future that it exceeds'
http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/8804/7/src/kudu/tserver/tablet_service.cc@1625
PS7, Line 1625: '
not your fault, but this looks like an extraneous quote.
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 01 Feb 2018 19:41:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ OWN WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_OWN_WRITES scan mode
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/common/common.proto
File src/kudu/common/common.proto:
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/common/common.proto@221
PS1, Line 221:
: // When BOUNDED_READ is specified, the server will pick an arbitrarily snapshot
: // in the past, subject to the propagated timestamp bound, and perform a read.
: // More specifically, the server will choose the newest timestamp within the
: // staleness bound that allows execution of the reads without blocking by the
: // in-flight transactions. If no propagated timestamp is provided the serv
> Not a big fan of BOUNDED_READ (bounded by what?) but I've had trouble to co
I would also prefer READ_OWN_WRITES than READ_BOUNDED_STALE as the later does not explain much by the name.
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/tserver/tablet_service.cc@2036
PS1, Line 2036: switch (scan_pb.read_mode()) {
: case READ_AT_SNAPSHOT:
: case READ_OWN_WRITES:
:
> nit: use switch case
Done
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/tserver/tablet_service.cc@2041
PS1, Line 2041: return Status::NotSupported("Unsupported snapshot scan mode specified");
: }
:
: // Based on the read mode, pick a timestamp and verify it.
: Timestamp tmp_snap_timestamp;
: RETURN_NOT_OK(PickAndVerifyTimeStamp(scan_pb, tablet_replica, &tmp_snap_timestamp));
:
: tablet::MvccSnapshot snap;
: Tablet* tablet = tablet_replica->tablet();
: scoped_refptr<consensus::TimeManager> time_manager = tablet_replica->time_manager();
: tablet::MvccManager* mvcc_manager = tablet->mvcc_manager();
:
: // Reduce the client's deadline by a few msecs to allow for overhead.
: MonoTime client_deadline = rpc_context->GetClientDeadline() - MonoDelta::FromMilliseconds(10);
:
: // Its not good for the tablet server or for the client if we hang here forever. The tablet
: // server will have one less available thread and the client might be stuck spending all
: // of the allotted time for the scan on a partitioned server that will never have a consistent
: // snapshot at 'snap_timestamp'.
: // Because of this we clamp the client's deadline to the max. configured. If the client
: // sets a long timeout then it can use it by trying in other servers.
: bool was_clamped = false;
: MonoTime final_deadline = ClampScanDeadlineForWait(client_deadline, &was_clamped);
:
: // Wait for the tablet to know that 'snap_timestamp' is safe. I.e. that all operations
: // that came before it are, at least, started. This, together with waiting for the mvcc
: // snapshot to be clean below, allows us to always return the same data when scanning at
: // the same timestamp (repeatable reads).
: TRACE("Waiting safe time to advance");
: MonoTime before = MonoTime::Now();
: Status s = time_manager->WaitUntilSafe(tmp_snap_timestamp, final_deadline);
:
: if (s.ok()) {
: // Wait for the in-flights in the snapshot to be finished.
: TRACE("Waiting for operations to commit");
: s = mvcc_manager->WaitForSnapshotWithAllCommitted(tmp_snap_timestamp, &snap, client_deadline);
: }
:
: // If we got an TimeOut but we had clamped the deadline, return a ServiceUnavailable instead
: // so that the client retries.
: if (s.IsTimedOut() && was_clamped) {
: return Status::ServiceUnavailable(s.CloneAndPrepend(
: "could not wait for desired snapshot timestamp to be consistent").ToString());
: }
: RETURN_NOT_OK(s);
:
: uint64_t duration_usec = (MonoTime::Now() - before).ToMicroseconds();
: tablet->metrics()->snapshot_read_inflight_wait_duration->Increment(duration_usec);
: TRACE("All operations in snapshot committed. Waited for $0 microseconds", duration_usec);
:
> Can you move all of this to a new method? maybe PickOrValidateTimestamp()
Done
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 13 Dec 2017 00:37:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 14: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 14
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 16 Feb 2018 18:58:54 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
KUDU-1704: add READ_YOUR_WRITES scan mode
This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode,
the server will pick a snapshot in the past, subject to the propagated
timestamp bound, and perform a read. Moreover, the chosen snapshot
timestamp is returned back to the client.
The major difference between READ_AT_SNAPSHOT scan without a timestamp
and READ_YOUR_WRITES scan is the latter will choose the newest timestamp
within the staleness bound that allows execution of the reads without
being blocked by the in-flight transactions if possible. READ_YOUR_WRITES
mode is not repeatable. However, it allows client local
read-your-writes/read-your-reads.
Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing
Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Reviewed-on: http://gerrit.cloudera.org:8080/8804
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mp...@apache.org>
Reviewed-by: David Ribeiro Alves <da...@gmail.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/common/common.proto
M src/kudu/tserver/tablet_server-test-base.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
6 files changed, 326 insertions(+), 98 deletions(-)
Approvals:
Kudu Jenkins: Verified
Mike Percy: Looks good to me, but someone else must approve
David Ribeiro Alves: Looks good to me, approved
Alexey Serbin: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 19
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 18: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 18
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 21 Feb 2018 18:37:43 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1953
PS10, Line 1953: // Make sure that there is no snapshot timestamp sent back.
: ASSERT_TRUE(!resp.has_snap_timestamp());
> I'm still unsure why we wouldn't the response to include the timestamp. I a
I see, so looks like you are proposing to make this read mode fault-tolerant as well. Which means we need to make it an ordered scan and ensure the client sent out the last primary key to retry on.
I do not see any reasons to not make this mode fault tolerant, but I do not see the reasons why we have to return the chosen snapshot timestamp for the scan to retry on another server in this case. I think the next tsever that the scan is retried on can chose the timestamp freely based on the conditions, right?
Moreover, would you mind I do another patch if we do want RYW to be fault tolerant, to make it more clear?
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Tue, 06 Feb 2018 01:21:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 12:
(23 comments)
http://gerrit.cloudera.org:8080/#/c/8804/12//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/8804/12//COMMIT_MSG@15
PS12, Line 15: choose the newest timestamp
: within the staleness bound that allows execution of the reads without
: being blocked by the in-flight transactions
> might be worth adding "if possible"
Done
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto
File src/kudu/common/common.proto:
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto@236
PS12, Line 236: subject to the propagated timestamp bound
> This wording is pretty hard to a casual reader (or even me) to understand.
Done
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto@237
PS12, Line 237: newest timestamp within
> latest timestamp higher than
Done
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto@242
PS12, Line 242: the timestamp
> a timestamp
Done
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto@244
PS12, Line 244: should use it
> Is this something that end-users have to worry about, or can we word this s
Updated.
Currently, we use current clock time of the server as the propagation time. This would bump up the propagation time unnecessarily for the next read in this mode.
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/common/common.proto@245
PS12, Line 245: wait
> nit: waiting
Done
http://gerrit.cloudera.org:8080/#/c/8804/11/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/8804/11/src/kudu/tserver/tablet_server-test.cc@185
PS11, Line 185: void ScanYourWritesTest(uint64_t propagated_timestamp, ScanResponsePB* resp);
> warning: parameter 'propagated_timestamp' is const-qualified in the functio
Done
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
PS12:
> We are missing a C++ client test in this patch. Is that excluded on purpose
C++ client test in the following CR https://gerrit.cloudera.org/#/c/8823/
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1924
PS12, Line 1924: TEST_F(TabletServerTest, TestScanYourWrites) {
> nit: add a comment at the top of this test describing what the test does
Done
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1924
PS12, Line 1924: TestScanYourWrites
> It worries me a bit that most tests of this functionality overwhelmingly te
Added more integration tests in C++ client which would definitely fail with READ_LATEST.
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1924
PS12, Line 1924: TestScanYourWrites
> I agree that a deterministic test would be nice and I think it should be po
Done
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1927
PS12, Line 1927: e
> nit: missing period
Done
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1958
PS12, Line 1958: TEST_F(TabletServerTest, TestScanYourWrites_WithoutPropagatedTimestamp) {
> nit: add test comment
Done
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1960
PS12, Line 1960: perform a write
> nit: use capitalization and punctuation for code comments per https://googl
Done
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_server-test.cc@1972
PS12, Line 1972: perform a write
> punctuation
Done
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.h
File src/kudu/tserver/tablet_service.h:
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.h@155
PS12, Line 155: // it exceeds the maximum allowed clock synchronization error time.
> Can we add a note about the context, i.e. why this matters?
Done
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.h@158
PS12, Line 158: if
> s/if/that/
Done
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2113
PS12, Line 2113:
> add: DCHECK(s.ok()) ?
I think even for status that returns 'NotSupported' can reach here.
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2114
PS12, Line 2114: // Note: if 'max_allowed_ts' is not obtained from clock_->GetGlobalLatest() it's guaranteed
: // to be higher than 'tmp_snap_timestamp'.
> I'm having trouble following this:
Good question. TBH, this is refactored from previous code, so I am not sure about the answers, but I will try my best.
My understanding of "not obtained" means the current clock type (logical clock) does not support this function. It returns 'NotSupported' status and FLAGS_scanner_allow_snapshot_scans_with_logical_timestamps is enabled.
For the second question, maybe David can shed some light?
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2133
PS12, Line 2133: that
> than
Done
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2135
PS12, Line 2135: much
> far
Done
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2164
PS12, Line 2164: RETURN_NOT_OK(ValidateTimeStamp(tmp_snap_timestamp));
> Hmm, we already updated the clock on line 2137 so AFAICT the only way this
Right, that is the purpose.
I think they are two different things. Updating the clock in L2137 is to ensure the server's clock is not lagging behind the others, while here is validating the chosen timestamp is not too far in the future.
http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2170
PS12, Line 2170: VerifyNotAncientHistory
> same here; is there some way to check this before updating the clock, or we
Same answer as above, it is only validating the chosen timestamp.
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 12
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Thu, 15 Feb 2018 01:21:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8804
to look at the new patch set (#9).
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
KUDU-1704: add READ_YOUR_WRITES scan mode
This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode,
the server will pick a snapshot in the past, subject to the propagated
timestamp bound, and perform a read. Moreover, the chosen snapshot
timestamp is returned back to the client.
The major difference between READ_AT_SNAPSHOT scan without a timestamp
and READ_YOUR_WRITES scan is the latter will choose the newest timestamp
within the staleness bound that allows execution of the reads without
being blocked by the in-flight transactions. READ_OWN_WRITES mode is not
repeatable. However, it allows client local read-your-writes.
Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing
Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
---
M src/kudu/common/common.proto
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
4 files changed, 260 insertions(+), 52 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/9
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 9
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Mike Percy, Dan Burkert, David Ribeiro Alves, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8804
to look at the new patch set (#13).
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
KUDU-1704: add READ_YOUR_WRITES scan mode
This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode,
the server will pick a snapshot in the past, subject to the propagated
timestamp bound, and perform a read. Moreover, the chosen snapshot
timestamp is returned back to the client.
The major difference between READ_AT_SNAPSHOT scan without a timestamp
and READ_YOUR_WRITES scan is the latter will choose the newest timestamp
within the staleness bound that allows execution of the reads without
being blocked by the in-flight transactions if possible. READ_YOUR_WRITES
mode is not repeatable. However, it allows client local
read-your-writes/read-your-reads.
Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing
Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
---
M src/kudu/common/common.proto
M src/kudu/tserver/tablet_server-test-base.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
6 files changed, 353 insertions(+), 107 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/13
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 13
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 11:
(13 comments)
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto
File src/kudu/common/common.proto:
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@222
PS10, Line 222: tion is
> nit: snapshot timestamp
Done
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@228
PS10, Line 228:
> nit: choose
Done
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@229
PS10, Line 229: serv
> nit: such that
Done
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@231
PS10, Line 231:
> The 'stale' terminology hasn't been introduced here, so I think it would be
Done
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@233
PS10, Line 233:
> I think it would be better to say 'different results' here in order to avoi
Done
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1922
PS10, Line 1922:
> We decided we also wanted scan your reads right? is that covered?
Done
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1944
PS10, Line 1944: ASSERT_EQ(R"((in
> I know this is likely copy/paste but remove (adds nothing)
Done
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1953
PS10, Line 1953: ASSERT_EQ(kNumRows, results.size());
: ASSERT_EQ(R"((int32 key=0, int32 int_val
> I think we discussed a couple of reasons why it might be interesting to hav
Right, updated it accordingly.
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1989
PS10, Line 1989: Hybrid
> same
Done
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.h
File src/kudu/tserver/tablet_service.h:
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.h@158
PS10, Line 158: ing to the scan m
> on the read mode?
Done
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.cc@2107
PS10, Line 2107: us s = server_->cl
> nit add a predict false
Done
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.cc@2161
PS10, Line 2161: )
> use kMinTimestamp or something? I understand you can't use kInvalidTimestam
Done
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.cc@2163
PS10, Line 2163: Timestamp(std::max(propagated_timestamp + 1, clean_timestamp));
: RETURN_NOT_OK(ValidateTimeStamp(tmp_snap_timestamp));
> std:max ?
Done
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 11
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Thu, 08 Feb 2018 04:36:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ OWN WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_OWN_WRITES scan mode
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/common/common.proto
File src/kudu/common/common.proto:
http://gerrit.cloudera.org:8080/#/c/8804/1/src/kudu/common/common.proto@228
PS1, Line 228: transactions before it are committed.
: //
: // Reads in this mode are not repeatable: two stale reads, even if they
: // provide the same propagated timestamp bound, can execute at different
: // timestamps and thus return inconsistent results. Ho
> makes sense. shall we preface this patch with that change then.
Yes, will do, thanks!
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 15 Dec 2017 21:43:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 16:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2163
PS16, Line 2163: (propagation ts + 1) should not
> As we update the clock based on propagated timestamp at L2140, if (propagat
Maybe we should just add that explanation to the docs?
Also: obvious while we have these things in context, but likely worth mentioning for the documentation value is that "clean" timestamp is, by definition in the past.
Suggestion, something like:
There is no need to validate if the chosen timestamp is too far in the future, as :
- MVCC's 'clean' ts is by definition in the past (it's maximally bounded by safe time).
- "propagated" ts was used to update the clock above and the update would have returned an error if the the timestamp was too far in the future.
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 16
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 21 Feb 2018 00:42:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Mike Percy, Dan Burkert, David Ribeiro Alves, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8804
to look at the new patch set (#14).
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
KUDU-1704: add READ_YOUR_WRITES scan mode
This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode,
the server will pick a snapshot in the past, subject to the propagated
timestamp bound, and perform a read. Moreover, the chosen snapshot
timestamp is returned back to the client.
The major difference between READ_AT_SNAPSHOT scan without a timestamp
and READ_YOUR_WRITES scan is the latter will choose the newest timestamp
within the staleness bound that allows execution of the reads without
being blocked by the in-flight transactions if possible. READ_YOUR_WRITES
mode is not repeatable. However, it allows client local
read-your-writes/read-your-reads.
Design doc: https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing
Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
---
M src/kudu/common/common.proto
M src/kudu/tserver/tablet_server-test-base.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
6 files changed, 320 insertions(+), 98 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/14
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 14
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 14:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:
http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.cc@2166
PS14, Line 2166: RETURN_NOT_OK(ValidateTimeStamp(tmp_snap_timestamp));
> What is this really checking. We need to validate the timestamp for READ_AT
Right, think a bit more, I do not see a case where this validation could fail (which essentially mean: propagation TS + 1 > propagation TS + FLAGS_max_clock_sync_error_usec)
So it may not worth checking here. I will remove.
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 14
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 16 Feb 2018 22:44:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1953
PS10, Line 1953: // Make sure that there is no snapshot timestamp sent back.
: ASSERT_TRUE(!resp.has_snap_timestamp());
> I see, so looks like you are proposing to make this read mode fault-toleran
I think we discussed a couple of reasons why it might be interesting to have the response include the timestamp (like using that to update the last propagated timestamp). I agree that fault-tolerance work doesn't need to be included in this patch though
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Tue, 06 Feb 2018 23:50:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1704: add READ OWN WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, David Ribeiro Alves, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8804
to look at the new patch set (#3).
Change subject: KUDU-1704: add READ_OWN_WRITES scan mode
......................................................................
KUDU-1704: add READ_OWN_WRITES scan mode
This patch adds a new read mode READ_OWN_WRITES on tserver. In this mode,
the server will pick an arbitrarily snapshot in the past, subject to
the propagated timestamp bound, and perform a read. Moreover, the chosen
snapshot timestamp is not returned back to the client.
The major difference between READ_AT_SNAPSHOT scan without a timestamp
and READ_OWN_WRITES scan is the latter will choose the newest timestamp
within the staleness bound that allows execution of the reads without
blocking by the in-flight transactions. READ_OWN_WRITES mode is not
repeatable. However, it allows client local read-your-writes.
Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
---
M src/kudu/common/common.proto
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
4 files changed, 262 insertions(+), 52 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/3
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8804 )
Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto
File src/kudu/common/common.proto:
http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@237
PS10, Line 237: READ_YOUR_WRITES = 3;
> It may be good to add docs about whether a snapshot timestamp and propagate
Will update.
We do not return the chosen snapshot timestamp, because as we discussed offline, for multi tablet scan we should not reuse the timestamps across servers in this mode. Each server should be free to choose a timestamp as long as it respects the aforementioned condition (This is documented in the design doc as well :)). The returned propagated timestamp is set to 'Now' of the tserver (same as other scan mode). However, as discussed the client should only update the propagated timestamp after the scan of last tablet for multi tablet scan.
--
To view, visit http://gerrit.cloudera.org:8080/8804
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 10
Gerrit-Owner: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Comment-Date: Fri, 02 Feb 2018 22:49:01 +0000
Gerrit-HasComments: Yes