Dan Burkert 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: (4 comments) http://gerrit.cloudera.org:8080/#/c/8823/17/src/kudu/client/scan_configuration.cc File src/kudu/client/scan_configuration.cc: http://gerrit.cloudera.org:8080/#/c/8823/17/src/kudu/client/scan_configuration.cc@45 PS17, Line 45: ScanConfiguration::ScanConfiguration(KuduTable* table) lower_bound_propagation_timestamp_ needs to be initialized, I'm surprised this didn't cause issues? 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: client->data_->UpdateLatestObservedTimestamp(message.propagated_timestamp()); > Hmm, currently SetScanLowerBoundTimestampRaw() is called when we open a kud Right, so KuduScanner::Open is called after this, and at that point it grabs the client's LatestObservedTimestamp and sets it as the scan config's propagation timestamp. That's correct, however I think it's stricter than necessary: we could get RYW by setting the scan config's propagation timestamp to the ScanToken's propagated timestamp, which may be older than the client's latest observed timestamp (e.g. if the client has other writes/scans happening concurrently). It may not matter in practice, though? http://gerrit.cloudera.org:8080/#/c/8823/10/src/kudu/client/scan_token-internal.cc@263 PS10, Line 263: pb.set_propagated_timestamp(client->GetLatestObservedTimestamp()); > Hmm, you mean use the previous scanner's propagated timestamp? I do not th yeah sorry I don't think this question made any sense. http://gerrit.cloudera.org:8080/#/c/8823/17/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: http://gerrit.cloudera.org:8080/#/c/8823/17/src/kudu/client/scanner-internal.cc@495 PS17, Line 495: } I think it would be simpler to only have the single branch like before, but check that in RYW the field is set: CHECK(configuration_.read_mode() != KuduScanner::READ_YOUR_WRITES || last_response_.has_snap_timestamp()); if (last_response_.has_propagated_timestamp()) { table_->client()->data_->UpdateLatestObservedTimestamp( last_response_.propagated_timestamp()); } -- 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-Reviewer: Tidy Bot Gerrit-Comment-Date: Fri, 02 Mar 2018 19:23:12 +0000 Gerrit-HasComments: Yes
