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 10: (17 comments) http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/client-test.cc@4436 PS10, Line 4436: kReadModes > hum, didn't you re-define this in LOC 871? Right, I can parametrize this test as well. http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/client.h@1740 PS10, Line 1740: the server will perform a server-local : /// snapshot scan that > "the server will perform a server-local snapshot scan" is hard to parse, pa Done http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/client.h@1742 PS10, Line 1742: each client > maybe remove "for each client" Done http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/client.h@1746 PS10, Line 1746: /// Reads in this mode are not repeatable: two READ_YOUR_WRITES reads, even if : /// they provide the same propagated timestamp bound, can execute at different : /// timestamps and thus return different results. > was trying to rephrase this but maybe we should remove this alltogether? Your concern is it is not good to mention this mode is not repeatable? http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/client.h@1750 PS10, Line 1750: /// In ACID terms this corresponds to Isolation mode: "Read Committed". > it seems weird that this has the same isolation mode as READ_LATEST. it's h In that case, maybe we should remove this one here? http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/client.cc@1203 PS10, Line 1203: if (data_->open_) { : return Status::IllegalState("Read mode must be set before Open()"); : } : if (!tight_enum_test<ReadMode>(read_mode)) { : return Status::InvalidArgument("Bad read mode"); : } : return data_->mutable_configuration()->SetReadMode(read_mode); > hum, regarding validation. IMO we should be permissive and let the user spe You are right. We should allow user specify a timestamp before setting the scan mode. I am update in the following patch to do validation later when open a scanner instead of in the config and when build scan tokens. http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/client.cc@1234 PS10, Line 1234: should > same doubt as above. Done http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/client.cc@1248 PS10, Line 1248: if (data_->configuration().read_mode() != READ_AT_SNAPSHOT) { : return Status::IllegalState("Snapshot timestamp should only be configured " : "for READ_AT_SNAPSHOT scan mode."); > same doubt as above. Done http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/client.cc@1621 PS10, Line 1621: return Status::IllegalState("Snapshot timestamp should only be configured " : "for READ_AT_SNAPSHOT scan mode."); > same Done http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/client.cc@1630 PS10, Line 1630: return Status::IllegalState("Snapshot timestamp should only be configured " : "for READ_AT_SNAPSHOT scan mode."); > same Done http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/scan_configuration.h File src/kudu/client/scan_configuration.h: http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/scan_configuration.h@89 PS10, Line 89: void SetPropagationTimestampRaw(uint64_t propagation_timestamp); > maybe SetScanLowerBoundTimestampRaw ? and then docs it's just for RYW ? (no Done 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@240 PS10, Line 240: Ignoring > well crashing is not "ignoring" :) Right, good catch! http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/scanner-internal.cc@318 PS10, Line 318: Ignoring > same Done http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/scanner-internal.cc@331 PS10, Line 331: Ignoring > same Done http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/scanner-internal.cc@356 PS10, Line 356: lo > nit: rename this var since this is no longer always the "last observed" Done 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@127 PS10, Line 127: ensures > s/ensures/ensure Done http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/integration-tests/consistency-itest.cc@746 PS10, Line 746: read-your-reads > how are you testing read your reads? Yes, in L765 do another scan to ensure read-your-reads. I will add a comment there. -- 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: 10 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: Wed, 21 Feb 2018 22:07:44 +0000 Gerrit-HasComments: Yes
