Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13535 )
Change subject: KUDU-2809 (3/6): C++ private API for diff scans ...................................................................... Patch Set 2: Code-Review+1 (3 comments) looks good modulo a couple nits and grant's suggestions http://gerrit.cloudera.org:8080/#/c/13535/2/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/13535/2/src/kudu/client/client.cc@1337 PS2, Line 1337: Status KuduScanner::SetDiffScan(uint64_t start_timestamp, uint64_t end_timestamp) { > Should we verify start is before end? Yeah, validating (start <= end) would be a good fit for config->SetDiffScan() along with the value validation done below http://gerrit.cloudera.org:8080/#/c/13535/2/src/kudu/client/client.cc@1737 PS2, Line 1737: if (start_timestamp == 0) { : return Status::IllegalState("Start timestamp must be set bigger than 0"); : } Maybe == kNoTimestamp is better (also equal to 0) http://gerrit.cloudera.org:8080/#/c/13535/2/src/kudu/client/client.cc@1740 PS2, Line 1740: if (end_timestamp == 0) { maybe just validate that start <= end ... also consider moving all of this into config->SetDiffScan() as suggested in another comment -- To view, visit http://gerrit.cloudera.org:8080/13535 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I03bd5e51c553b239e36fbdfc4a94131c9599851e Gerrit-Change-Number: 13535 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Fri, 07 Jun 2019 01:51:41 +0000 Gerrit-HasComments: Yes
