Abhishek Chennaka 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: (18 comments) http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/client/client-test.cc@9650 PS6, Line 9650: TestDiffScanReinsertReportedLive > nit for here and elsewhere: there is no need to add 'Test' as the prefix fo Majority of the tests in ClientTest suite seem to be having this convention. I removed it for these tests now. http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/client/client-test.cc@9708 PS6, Line 9708: TestDiffScanReinsertViaUpsertThenDeleteReportedDeleted > It seems this scenario consistently fails in every build configuration (DEB Fixed it. http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/client/client-test.cc@9728 PS6, Line 9728: TestDiffScanIncludeUnobservableRows > It seems this scenario consistently fails in every build configuration (DEB Fixed it 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 st Done http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/24578/6/src/kudu/client/client.cc@1925 PS6, Line 1925: visibility == DiffScanRowVisibility::OBSERVABLE_ONLY ); > Should this be `visibility == INCLUDE_UNOBSERVABLE`? As written, Yes, thanks for pointing out the error. 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 h Done 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 continu Done 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 func Done 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 Done 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 par Done 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 paramete Done 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? Done 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 Done 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 unders Done 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 ( Yes, removed it. 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? Done 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 hav Done 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 Done -- 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: Marton Greber <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Mon, 10 Aug 2026 01:35:50 +0000 Gerrit-HasComments: Yes
