Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/24578 )
Change subject: KUDU-3800 Add diff-scan option to return unobservable rows ...................................................................... Patch Set 12: (8 comments) http://gerrit.cloudera.org:8080/#/c/24578/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24578/12//COMMIT_MSG@30 PS12, Line 30: include_unobservable_rows > here and elsewhere: 'include_unobservable_rows' isn't present in NewScanReq Done http://gerrit.cloudera.org:8080/#/c/24578/12/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/24578/12/src/kudu/client/client.h@3150 PS12, Line 3150: Status SetDiffScan(uint64_t start_timestamp, uint64_t end_timestamp) : WARN_UNUSED_RESULT KUDU_NO_EXPORT; > After revisiting changes in PS12, I just realized this flavor of the SetDif Done - There is only one SetDiffScan() with three arguments. Call sites have been updated accordingly. http://gerrit.cloudera.org:8080/#/c/24578/12/src/kudu/client/client.proto File src/kudu/client/client.proto: http://gerrit.cloudera.org:8080/#/c/24578/12/src/kudu/client/client.proto@172 PS12, Line 172: // See kudu.tserver.NewScanRequestPB.row_visibility. When set to : // INCLUDE_UNOBSERVABLE, the emitting client must also add : // ScanTokenPB.Feature::RowVisibility to feature_flags so that a : // deserializing client without this field's semantics fails cleanly. : optional RowVisibility row_visibility = 26 [default = OBSERVABLE_ONLY]; > It would be nice to cover how this new field goes through serialization --> Done - Tests added in scan_token-test.cc http://gerrit.cloudera.org:8080/#/c/24578/12/src/kudu/client/scan_configuration.cc File src/kudu/client/scan_configuration.cc: http://gerrit.cloudera.org:8080/#/c/24578/12/src/kudu/client/scan_configuration.cc@65 PS12, Line 65: row_format_flags_(KuduScanner::NO_FLAGS), : row_visibility_(KuduScanner::OBSERVABLE_ONLY) { > Aren't there reversed compared with their declaration order in the header f Oops, yes - fixed it now http://gerrit.cloudera.org:8080/#/c/24578/12/src/kudu/tablet/diff_scan-test.cc File src/kudu/tablet/diff_scan-test.cc: http://gerrit.cloudera.org:8080/#/c/24578/12/src/kudu/tablet/diff_scan-test.cc@174 PS12, Line 174: opts.snap_to_include = snap_to_include; > nit: add std::move() to avoid extra copying? Done http://gerrit.cloudera.org:8080/#/c/24578/12/src/kudu/tablet/diff_scan-test.cc@175 PS12, Line 175: opts.order = ORDERED; > IIUC, in the new test scenarios the only covered ordering option is ORDERED Done - Added UnobservableRowsMemRowSet to cover this scenario. http://gerrit.cloudera.org:8080/#/c/24578/12/src/kudu/tablet/diff_scan-test.cc@483 PS12, Line 483: SCOPED_TRACE(rv == INCLUDE_UNOBSERVABLE ? "INCLUDE_UNOBSERVABLE" : : "OBSERVABLE_ONLY"); > Here and elsewhere: is it possible to use the auto-generated RowVisibility_ Done http://gerrit.cloudera.org:8080/#/c/24578/12/src/kudu/tablet/diff_scan-test.cc@520 PS12, Line 520: > These newly added synthetic test scenarios always use a single row key. Do Done - Added OrderedDiffScanWithDeletesTest.MixedRowLifecycles to cover these cases -- 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: 12 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-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Thu, 27 Aug 2026 18:57:22 +0000 Gerrit-HasComments: Yes
