Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24578 )
Change subject: Add diff-scan option to return unobservable rows ...................................................................... Patch Set 6: (14 comments) http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/client/client.h@3154 PS6, Line 3154: enum class I don't think this can be accepted by a C++98-compiant C++ compiler. We still have to provide C++98 compatibility for our C++ client API since it has been declared originally and the Kudu project hasn't yet officially dropped support for C++98 in Kudu C++ client API. http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/client/client.proto File src/kudu/client/client.proto: http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/client/client.proto@168 PS6, Line 168: NewScanRequestPB.include_unobservable_rows nit: maybe, also include the notion of the different package kudu.tserver here (since this is kudu.client) http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/client/scan_token-internal.cc File src/kudu/client/scan_token-internal.cc: http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/client/scan_token-internal.cc@302 PS6, Line 302: ? KuduScanner::DiffScanRowVisibility::INCLUDE_UNOBSERVABLE : : KuduScanner::DiffScanRowVisibility::OBSERVABLE_ONLY; style nit: fix the indent -- these lines aren't new lines, they are continuation of the line above and should have different indent http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/tablet/diff_scan-test.cc File src/kudu/tablet/diff_scan-test.cc: http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/tablet/diff_scan-test.cc@167 PS6, Line 167: DoScan style nit: move this to come before all other non-static public member functions and constructors/destructors of the class http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/tablet/diff_scan-test.cc@167 PS6, Line 167: std::shared_ptr<Tablet> tablet style nit: move this into its separate line as well http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/tablet/diff_scan-test.cc@168 PS6, Line 168: MvccSnapshot snap1, : MvccSnapshot snap2, nit: user names that reflect better on the semantics of these these two parameters http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/tablet/diff_scan-test.cc@171 PS6, Line 171: * style nit: the asterisk should be with the type, not the name of a parameter http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/tablet/diff_scan-test.cc@178 PS6, Line 178: const nit: is it possible to use constexpr here? http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/tablet/diff_scan-test.cc@245 PS6, Line 245: TestUnobservableRowsMemRowSet nit for there and elsewhere: there is no need to include 'Test' prefix into the name of the scenario -- it's already in the name of the test itself http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/tablet/diff_scan-test.cc@259 PS6, Line 259: true nit for here and elsewhere: add /*include_unobservable=*/ for easier understanding of what this parameter means? http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/tablet/memrowset.cc File src/kudu/tablet/memrowset.cc: http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/tablet/memrowset.cc@713 PS6, Line 713: opts_.snap_to_exclude Isn't this always true because of the first criteria of the enclosing 'if ()' scope? http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/tablet/rowset.h File src/kudu/tablet/rowset.h: http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/tablet/rowset.h@106 PS6, Line 106: iteration Typo, perhaps? http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/tablet/rowset.h@116 PS6, Line 116: = false; nit: better move this into the initializer's list in the constructor to have uniformity with the rest of boolean fields in RowIteratorOptions http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/tserver/tablet_service.cc@3035 PS6, Line 3035: scan_pb.include_unobservable_rows() && !scan_pb.has_snap_start_timestamp() nit: consider wrapping this into PREDICT_FALSE() since we aren't expecting such mistakes in the Kudu C++ client library at least. -- To view, visit http://gerrit.cloudera.org:8080/24578 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0d0727fb2b89c727c669ec218e3bb8594336359a Gerrit-Change-Number: 24578 Gerrit-PatchSet: 6 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Mon, 03 Aug 2026 21:40:07 +0000 Gerrit-HasComments: Yes
