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 Gerrit-MessageType: comment Gerrit-Change-Id: I4d79024b088ea88fd194cabcb61e640f66326264 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
