David Ribeiro Alves 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? 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, particularly as these are user facing docs. maybe: "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:" http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/client.h@1742 PS10, Line 1742: each client maybe remove "for each client" 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? 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 hard to map to SQL isolation modes as they were definitely created with single-node systems and locks in mind. maybe leave "Read Committed" here but remove it from 'Read Latest"? 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 specify a timestamp before setting the scan mode. This means having to adjust validation here and below (e.g. can't have a timestamp for a mode other than RAS). thoughts? http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/client.cc@1234 PS10, Line 1234: should same doubt as above. 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. 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 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 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 ? (not your fault, but maybe also doc the two above very briefly) 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" :) 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 http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/scanner-internal.cc@331 PS10, Line 331: Ignoring same 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" 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 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? -- 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 19:41:34 +0000 Gerrit-HasComments: Yes
