Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8823 )
Change subject: KUDU-1704: add c++ client support for READ_YOUR_WRITES mode ...................................................................... Patch Set 12: (6 comments) http://gerrit.cloudera.org:8080/#/c/8823/12/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/8823/12/src/kudu/client/client.h@1740 PS12, Line 1740: When @c READ_YOUR_WRITES is specified, the client will perform a read : /// such that it follows all previously known writes and reads from this client. : /// Specifically this mode: > this is great, thanks for making those changes. Thank you for the helpful comments! http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/scan_token-internal.cc File src/kudu/client/scan_token-internal.cc: http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/scan_token-internal.cc@186 PS10, Line 186: } > Does this need to be updated to also call SetPropagationTimestampRaw for a Hmm, currently SetScanLowerBoundTimestampRaw() is called when we open a kuduscanner which is after this call, so not quite follow why it would be too strict? http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/scan_token-internal.cc@263 PS10, Line 263: > Should this be the scanner's propagated timestamp for RYW mode? Otherwise Hmm, you mean use the previous scanner's propagated timestamp? I do not think we can use that since the chosen snapshot timestamp is bigger than that. http://gerrit.cloudera.org:8080/#/c/8823/12/src/kudu/client/scan_token-internal.cc File src/kudu/client/scan_token-internal.cc: http://gerrit.cloudera.org:8080/#/c/8823/12/src/kudu/client/scan_token-internal.cc@239 PS12, Line 239: IllegalState > Consider making this InvalidArgument, since it's an error caused by the arg Done http://gerrit.cloudera.org:8080/#/c/8823/12/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: http://gerrit.cloudera.org:8080/#/c/8823/12/src/kudu/client/scanner-internal.cc@365 PS12, Line 365: scan->set_propagated_timestamp(ts); > This isn't changing so not an issue, but I'm curious if this is setting the Right for READ_LATEST mode the propagation timestamp is ignored. http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/integration-tests/consistency-itest.cc File src/kudu/integration-tests/consistency-itest.cc: http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/integration-tests/consistency-itest.cc@746 PS10, Line 746: read-your-reads > did you forget to add the comment? can't see it on the last rev. Oops, sorry I forgot to add the comments. See your points now, updated. -- To view, visit http://gerrit.cloudera.org:8080/8823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I34214245a78aed172a28fbdb395ff5bccd0fc0e1 Gerrit-Change-Number: 8823 Gerrit-PatchSet: 12 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Alex Rodoni <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Comment-Date: Fri, 23 Feb 2018 22:29:18 +0000 Gerrit-HasComments: Yes
