[kudu-CR] KUDU-1704: add c++ client support for READ YOUR WRITES mode

["Dan Burkert (Code Review)" <ge...@cloudera.org>]

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 <ha...@cloudera.com>
Gerrit-Reviewer: Alex Rodoni <ar...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 02 Mar 2018 19:23:12 +0000
Gerrit-HasComments: Yes