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 <aser...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
