Alexey Serbin has posted comments on this change. Change subject: WIP: KUDU-1189 if not set, use timestamp from first server ......................................................................
Patch Set 1: (3 comments) Thank you for the review! http://gerrit.cloudera.org:8080/#/c/5143/1/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: PS1, Line 408: configuration_.snapshot_timestamp() != ScanConfiguration::kNoTimestamp > in general, does this cover the case where the user didn't specify a timest Those scans are sent to tablet servers sequentially, right? I.e. we are not sending out requests to different tablet servers in parallel when doing scan, so first we are about to receive response from the first server before we are sending request to next one, correct? If so, then the response from the first server will hit this code first and set the timestamp into the scan configuration. Next scan requests will just re-use the timestamp which is set. Do I miss something? PS1, Line 408: configuration_.snapshot_timestamp() != ScanConfiguration::kNoTimestam > maybe add a has_snapshot_timestamp() to scan_configuration? yep, that's done already in my other patch. Will need to re-base. PS1, Line 409: last_response_.has_snap_timestamp() > when would this not be set? Not sure what to do there: maybe crash? maybe r As I understood from the tserver.proto, that's not set in the response when it's READ_AT_SNAPSHOT scan and the corresponding request contained the snapshot timestamp set. -- To view, visit http://gerrit.cloudera.org:8080/5143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2ac708e38b8a80834f7d54eca294517cbfb06ec6 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
