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 8: (3 comments) http://gerrit.cloudera.org:8080/#/c/8823/8/src/kudu/client/client-internal.h File src/kudu/client/client-internal.h: http://gerrit.cloudera.org:8080/#/c/8823/8/src/kudu/client/client-internal.h@197 PS8, Line 197: uint64_t GetMaxObservedTimestamp() const; Please add docs to these, since it's not obvious anymore, e.g. what's the difference between GetLatestObserved and GetMaxObserverd http://gerrit.cloudera.org:8080/#/c/8823/8/src/kudu/client/client-internal.h@278 PS8, Line 278: AtomicInt<uint64_t> max_observed_timestamp_; I didn't expect an additional field on the client here, instead I though the scanner would get a new field 'propagated_timestamp' that would be used in RYW scans. Instead of using the client's latest_observed_timestamp() as the propagated timestamp it would use that field. As implemented here, isn't there interactions between two concurrently opened RYW scanners? http://gerrit.cloudera.org:8080/#/c/8823/8/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/8823/8/src/kudu/client/client.h@1740 PS8, Line 1740: /// When @c READ_YOUR_WRITES is specified, the server will pick a snapshot timestamp We should consider 'dumbing down' these docs considerably. For instance, the client really shouldn't need to know anything about timestamps or in-flight transactions to use or pick this scan mode. I think it's fine to get into this level of specificity on the READ_YOUR_WRITES docs in the .proto, since that's internal, however these docs have a different audience. I'd try and re-focus this doc on the characteristics of the scan from the perspective of the client application. I'd also consider asking Alexandra to do do a pass on this. -- 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: 8 Gerrit-Owner: Hao Hao <[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-Comment-Date: Fri, 16 Feb 2018 19:36:02 +0000 Gerrit-HasComments: Yes
