You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2016/11/15 23:40:35 UTC

[kudu-CR] [c++client] propagating timestamp for scans

Alexey Serbin has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/5099

Change subject: [c++client] propagating timestamp for scans
......................................................................

[c++client] propagating timestamp for scans

KUDU-1679 Propagate timestamps for scans

Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
---
M src/kudu/client/scan_configuration.cc
M src/kudu/client/scan_configuration.h
M src/kudu/client/scanner-internal.cc
3 files changed, 63 insertions(+), 14 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/5099/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>

[kudu-CR] KUDU-1679 Propagate timestamps for scans

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1679 Propagate timestamps for scans
......................................................................


Patch Set 5:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5099/5//COMMIT_MSG
Commit Message:

PS5, Line 10: set by the tablet server
s/set by the tablet server/set by the tablet server to the current clock value when a NewScanRequestPB message is processed successfully.


http://gerrit.cloudera.org:8080/#/c/5099/5/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

PS5, Line 594: CountRows
CountRows->CountRowsOnLeaders?


http://gerrit.cloudera.org:8080/#/c/5099/5/src/kudu/client/scan_token-internal.cc
File src/kudu/client/scan_token-internal.cc:

PS5, Line 217: READ_LATEST
s/READ_LATEST/not in READ_AT_SNAPSHOT


http://gerrit.cloudera.org:8080/#/c/5099/5/src/kudu/client/scanner-internal.cc
File src/kudu/client/scanner-internal.cc:

PS5, Line 295: if (configuration_.has_snapshot_timestamp()) {
             :     if (PREDICT_TRUE(read_mode == KuduScanner::READ_AT_SNAPSHOT)) {
             :       scan->set_snap_timestamp(configuration_.snapshot_timestamp());
             :     } else {
             :       LOG(WARNING) << "Ignoring snapshot timestamp since in READ_LATEST mode.";
             :     }
             :   }
maybe do this within the switch block above?


-- 
To view, visit http://gerrit.cloudera.org:8080/5099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [c++client] propagating timestamp for scans

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

David Ribeiro Alves has posted comments on this change.

Change subject: [c++client] propagating timestamp for scans
......................................................................

Patch Set 1:

I mention this yesterday, but it was pretty late and maybe you didn't get a chance to read it.
A snapshot's timestamp is distinct from the timestamp that is propagated. If you look at NewScanRequestPB (https://github.com/cloudera/kudu/blob/master/src/kudu/tserver/tserver.proto#L209)
you can see that there is a snap_timestamp field and propagated_timestamp field.
We already propagate the client's timestamp to the server (when we set 'snap_timestamp' in NewScanRequestPB, what we're missing is getting a 'propagated_timestamp' in ScanResponsePB and using that to update the client's last known timestamp.

The snap_timestamp in the ScanRequest/Response refers to the timestamp of the scan itself. For instance the user can set this arbitrarily in the past to do historic scans, but the propagated_timestamp would still be the current value of the servers clock.
I see how having a 'snap_timestamp' in the response might be confusing, but it does refer to the scan's timestamp and not to the timestamp that must be propagated. It's there because if the user doesn't specify a timestamp for the scan the server chooses one (based on the value obtained from the clock if after being updated with the ScanRequest's propagated timestamp). The value is returned on the response so that the client can continue the scan on another tablet while using the same timestamp.

--
To view, visit http://gerrit.cloudera.org:8080/5099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] KUDU-1679 Propagate timestamps for scans

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1679 Propagate timestamps for scans
......................................................................


Patch Set 7: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/5099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] [c++client] propagating timestamp for scans

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.

Todd Lipcon has posted comments on this change.

Change subject: [c++client] propagating timestamp for scans
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5099/2//COMMIT_MSG
Commit Message:

Line 7: [c++client] propagating timestamp for scans
nit: can you put the JIRA in the first line of the commit message? eg KUDU-1679: [c++-client] propagate timestamp for scans


-- 
To view, visit http://gerrit.cloudera.org:8080/5099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1679 Propagate timestamps for scans

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5099

to look at the new patch set (#4).

Change subject: KUDU-1679 Propagate timestamps for scans
......................................................................

KUDU-1679 Propagate timestamps for scans

Added the 'propagated_timestamp' field into the ScanResponsePB message.
It's always set by the tablet server which processed the incoming
NewScanRequestPB message successfully.

Also, added a unit test to verify the presence of the field in tablet
server response messages.

Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
---
M python/kudu/tests/test_scanner.py
M src/kudu/client/client-test.cc
M src/kudu/client/scan_configuration.cc
M src/kudu/client/scan_configuration.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
9 files changed, 197 insertions(+), 45 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/5099/4
-- 
To view, visit http://gerrit.cloudera.org:8080/5099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1679 Propagate timestamps for scans

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Alexey Serbin has posted comments on this change.

Change subject: KUDU-1679 Propagate timestamps for scans
......................................................................


Patch Set 5:

> Could you put this patch after the consistency itest patch in the
 > git history and enable that test here?

Good idea.  Sure, will do.

-- 
To view, visit http://gerrit.cloudera.org:8080/5099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] KUDU-1679 Propagate timestamps for scans

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1679 Propagate timestamps for scans
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5099/4/src/kudu/client/scan_configuration.h
File src/kudu/client/scan_configuration.h:

PS4, Line 166: uint64_t snapshot_timestamp_;
> For C++ mapping the PB field is uint64:
good point. dunno why I thought it mapped to int64


-- 
To view, visit http://gerrit.cloudera.org:8080/5099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1679 Propagate timestamps for scans

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5099

to look at the new patch set (#6).

Change subject: KUDU-1679 Propagate timestamps for scans
......................................................................

KUDU-1679 Propagate timestamps for scans

Added the 'propagated_timestamp' field into the ScanResponsePB message.
It's always set by the tablet server which processed the incoming
NewScanRequestPB message successfully.

Also, added a unit test to verify the presence of the field in tablet
server response messages.

Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
---
M src/kudu/client/client-test.cc
M src/kudu/client/scan_configuration.cc
M src/kudu/client/scan_configuration.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
8 files changed, 203 insertions(+), 56 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/5099/6
-- 
To view, visit http://gerrit.cloudera.org:8080/5099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1679 Propagate timestamps for scans

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1679 Propagate timestamps for scans
......................................................................


Patch Set 5:

Could you put this patch after the consistency itest patch in the git history and enable that test here?

-- 
To view, visit http://gerrit.cloudera.org:8080/5099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] KUDU-1679 Propagate timestamps for scans

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Alexey Serbin has posted comments on this change.

Change subject: KUDU-1679 Propagate timestamps for scans
......................................................................


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5099/5/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

PS5, Line 594: CountRows
> CountRows->CountRowsOnLeaders?
Done


http://gerrit.cloudera.org:8080/#/c/5099/5/src/kudu/client/scan_token-internal.cc
File src/kudu/client/scan_token-internal.cc:

PS5, Line 217: READ_LATEST
> s/READ_LATEST/not in READ_AT_SNAPSHOT
Done


http://gerrit.cloudera.org:8080/#/c/5099/5/src/kudu/client/scanner-internal.cc
File src/kudu/client/scanner-internal.cc:

PS5, Line 295: if (configuration_.has_snapshot_timestamp()) {
             :     if (PREDICT_TRUE(read_mode == KuduScanner::READ_AT_SNAPSHOT)) {
             :       scan->set_snap_timestamp(configuration_.snapshot_timestamp());
             :     } else {
             :       LOG(WARNING) << "Ignoring snapshot timestamp since in READ_LATEST mode.";
             :     }
             :   }
> maybe do this within the switch block above?
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/5099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1679 Propagate timestamps for scans

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Alexey Serbin has posted comments on this change.

Change subject: KUDU-1679 Propagate timestamps for scans
......................................................................


Patch Set 2:

(1 comment)

> I mention this yesterday, but it was pretty late and maybe you
 > didn't get a chance to read it.
 > A snapshot's timestamp is distinct from the timestamp that is
 > propagated. If you look at NewScanRequestPB (https://github.com/cloudera/kudu/blob/master/src/kudu/tserver/tserver.proto#L209)
 > you can see that there is a snap_timestamp field and
 > propagated_timestamp field.
 > We already propagate the client's timestamp to the server (when we
 > set 'snap_timestamp' in NewScanRequestPB, what we're missing is
 > getting a 'propagated_timestamp' in ScanResponsePB and using that
 > to update the client's last known timestamp.
 > 
 > The snap_timestamp in the ScanRequest/Response refers to the
 > timestamp of the scan itself. For instance the user can set this
 > arbitrarily in the past to do historic scans, but the
 > propagated_timestamp would still be the current value of the
 > servers clock.
 > I see how having a 'snap_timestamp' in the response might be
 > confusing, but it does refer to the scan's timestamp and not to the
 > timestamp that must be propagated. It's there because if the user
 > doesn't specify a timestamp for the scan the server chooses one
 > (based on the value obtained from the clock if after being updated
 > with the ScanRequest's propagated timestamp). The value is returned
 > on the response so that the client can continue the scan on another
 > tablet while using the same timestamp.

Thank you for the explanation.  For some reason, I confused the propagated timestamp and scan timestamp.  Posted the updated patch.

http://gerrit.cloudera.org:8080/#/c/5099/2//COMMIT_MSG
Commit Message:

Line 7: [c++client] propagating timestamp for scans
> nit: can you put the JIRA in the first line of the commit message? eg KUDU-
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/5099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1679 Propagate timestamps for scans

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5099

to look at the new patch set (#7).

Change subject: KUDU-1679 Propagate timestamps for scans
......................................................................

KUDU-1679 Propagate timestamps for scans

Added the 'propagated_timestamp' field into the ScanResponsePB message.
It's always set by the tablet server which processed the incoming
NewScanRequestPB message successfully.

Also, added a unit test to verify the presence of the field in tablet
server response messages.

Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
---
M src/kudu/client/client-test.cc
M src/kudu/client/scan_configuration.cc
M src/kudu/client/scan_configuration.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/integration-tests/consistency-itest.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
9 files changed, 204 insertions(+), 57 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/5099/7
-- 
To view, visit http://gerrit.cloudera.org:8080/5099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1679 Propagate timestamps for scans

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5099

to look at the new patch set (#3).

Change subject: KUDU-1679 Propagate timestamps for scans
......................................................................

KUDU-1679 Propagate timestamps for scans

Added the 'propagated_timestamp' field into the ScanResponsePB message.
It's always set by the tablet server which processed the incoming
NewScanRequestPB message successfully.

Also, added a unit test to verify the presence of the field in tablet
server response messages.

Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
---
M src/kudu/client/client-test.cc
M src/kudu/client/scan_configuration.cc
M src/kudu/client/scan_configuration.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
8 files changed, 104 insertions(+), 41 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/5099/3
-- 
To view, visit http://gerrit.cloudera.org:8080/5099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-1679 Propagate timestamps for scans

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

David Ribeiro Alves has posted comments on this change.

Change subject: KUDU-1679 Propagate timestamps for scans
......................................................................

Patch Set 4:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/5099/4/python/kudu/tests/test_scanner.py
File python/kudu/tests/test_scanner.py:

Line 194: .set_selection(kudu.LEADER_ONLY)\
were the python tests failing after this change?

http://gerrit.cloudera.org:8080/#/c/5099/4/src/kudu/client/scan_configuration.h
File src/kudu/client/scan_configuration.h:

PS4, Line 166: uint64_t snapshot_timestamp_;
why the change to uint? isn't the PB field an int64?

http://gerrit.cloudera.org:8080/#/c/5099/4/src/kudu/client/scan_token-internal.cc
File src/kudu/client/scan_token-internal.cc:

PS4, Line 209: PREDICT_TRUE(configuration_.read_mode() == KuduScanner::READ_LATEST
why are we setting a snap timestamp on a READ_LATEST scan? Also this seems to go against the warning below. Did you mean to use READ_AT_SNAPSHOT here. Finally can we leave the scan token changes to another patch? this is a separate matter and I was pondering with MJ whether it was worth to keep the timestamp inside the scan token itself.

http://gerrit.cloudera.org:8080/#/c/5099/4/src/kudu/client/scanner-internal.cc
File src/kudu/client/scanner-internal.cc:

PS4, Line 415: snapshoty
typo

PS4, Line 414: // TODO(aserbin): clarify whether it's OK to override the explicitly set
: // snapshoty timestamp with the timestamp from the last response.
this is valid. This basically makes retries of scans use the scan of the last response. It's not supposed to be overriding anything. If the user did provide a timestamp then 'snap_timestamp' on the response should be that timestmap. If the user didn't provide one then it should be the timestamp chosen by the server. In either case we're not overriding anything by setting it here.

http://gerrit.cloudera.org:8080/#/c/5099/4/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:

PS4, Line 1156: // make sure that the snapshot timestamp that was selected is >= now
can you additionally make sure that the propagated timestamp is after the scan's timestamp?

http://gerrit.cloudera.org:8080/#/c/5099/4/src/kudu/tserver/tserver.proto
File src/kudu/tserver/tserver.proto:

PS4, Line 351: nder the hood,
: // the client is supposed to copy the value from this field into the
: // WriteRequestPB::propagated_timestamp and/or
: // NewScanRequestPB::propagated_timestamp fields to achieve consistency in its
: // read- and write-operations.
this comment goes a bit too much into the client impl, IMO. Maybe mention this where we already introduce other consistency stuff. I think we talk a bit about it in the ReadModes definition.

--
To view, visit http://gerrit.cloudera.org:8080/5099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1679 Propagate timestamps for scans

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5099

to look at the new patch set (#5).

Change subject: KUDU-1679 Propagate timestamps for scans
......................................................................

KUDU-1679 Propagate timestamps for scans

Added the 'propagated_timestamp' field into the ScanResponsePB message.
It's always set by the tablet server which processed the incoming
NewScanRequestPB message successfully.

Also, added a unit test to verify the presence of the field in tablet
server response messages.

Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
---
M src/kudu/client/client-test.cc
M src/kudu/client/scan_configuration.cc
M src/kudu/client/scan_configuration.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
8 files changed, 199 insertions(+), 50 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/5099/5
-- 
To view, visit http://gerrit.cloudera.org:8080/5099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [c++client] propagating timestamp for scans

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Alexey Serbin has uploaded a new patch set (#2).

Change subject: [c++client] propagating timestamp for scans
......................................................................

[c++client] propagating timestamp for scans

KUDU-1679 Propagate timestamps for scans

Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
---
M src/kudu/client/scan_configuration.cc
M src/kudu/client/scan_configuration.h
M src/kudu/client/scanner-internal.cc
3 files changed, 62 insertions(+), 14 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/99/5099/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-1679 Propagate timestamps for scans

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Alexey Serbin has posted comments on this change.

Change subject: KUDU-1679 Propagate timestamps for scans
......................................................................

Patch Set 4:

(7 comments)

Thank you for the review!

I'm sending in a new version in a moment.

http://gerrit.cloudera.org:8080/#/c/5099/4/python/kudu/tests/test_scanner.py
File python/kudu/tests/test_scanner.py:

Line 194: .set_selection(kudu.LEADER_ONLY)\
> were the python tests failing after this change?
No, they failed prior to that. I added that to reflect the recommended sequence for RYW behavior.

The Python tests failed due to typo I did at scantoken code -- not setting the timestamp for READ_AT_SNASHOT reads.

I had some issues running Python test on my laptop, so that was a change in hope to fix the issue. Now those issues are resolved and I can run Python tests. Everything looks green after I fixed that typo in the scantoken code -- I'll rollback these changes in Python tests.

http://gerrit.cloudera.org:8080/#/c/5099/4/src/kudu/client/scan_configuration.h
File src/kudu/client/scan_configuration.h:

PS4, Line 166: uint64_t snapshot_timestamp_;
> why the change to uint? isn't the PB field an int64?
For C++ mapping the PB field is uint64:

https://developers.google.com/protocol-buffers/docs/proto

Besides, it matches with other client methods such as GetLatestObservedTimestamp() and other which return timestamp as unixtime micro.

http://gerrit.cloudera.org:8080/#/c/5099/4/src/kudu/client/scan_token-internal.cc
File src/kudu/client/scan_token-internal.cc:

PS4, Line 209: PREDICT_TRUE(configuration_.read_mode() == KuduScanner::READ_LATEST
> why are we setting a snap timestamp on a READ_LATEST scan? Also this seems
Great catch! That's a mistake -- I tried to remove double-negation and did a mistake.

http://gerrit.cloudera.org:8080/#/c/5099/4/src/kudu/client/scanner-internal.cc
File src/kudu/client/scanner-internal.cc:

PS4, Line 415: snapshoty
> typo
Done

PS4, Line 414: // TODO(aserbin): clarify whether it's OK to override the explicitly set
: // snapshoty timestamp with the timestamp from the last response.
> this is valid. This basically makes retries of scans use the scan of the la
Thank you for the explanation. I will remove this TODO since it's clarified.

http://gerrit.cloudera.org:8080/#/c/5099/4/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:

PS4, Line 1156: // make sure that the snapshot timestamp that was selected is >= now
> can you additionally make sure that the propagated timestamp is after the s
Done

http://gerrit.cloudera.org:8080/#/c/5099/4/src/kudu/tserver/tserver.proto
File src/kudu/tserver/tserver.proto:

PS4, Line 351: nder the hood,
: // the client is supposed to copy the value from this field into the
: // WriteRequestPB::propagated_timestamp and/or
: // NewScanRequestPB::propagated_timestamp fields to achieve consistency in its
: // read- and write-operations.
> this comment goes a bit too much into the client impl, IMO. Maybe mention t
Done

--
To view, visit http://gerrit.cloudera.org:8080/5099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

[kudu-CR] KUDU-1679 Propagate timestamps for scans

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.

David Ribeiro Alves has submitted this change and it was merged.

Change subject: KUDU-1679 Propagate timestamps for scans
......................................................................


KUDU-1679 Propagate timestamps for scans

Added the 'propagated_timestamp' field into the ScanResponsePB message.
It's always set by the tablet server which processed the incoming
NewScanRequestPB message successfully.

Also, added a unit test to verify the presence of the field in tablet
server response messages.

Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
Reviewed-on: http://gerrit.cloudera.org:8080/5099
Tested-by: Kudu Jenkins
Reviewed-by: David Ribeiro Alves <dr...@apache.org>
---
M src/kudu/client/client-test.cc
M src/kudu/client/scan_configuration.cc
M src/kudu/client/scan_configuration.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/integration-tests/consistency-itest.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tserver.proto
9 files changed, 204 insertions(+), 57 deletions(-)

Approvals:
  David Ribeiro Alves: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/5099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>