Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/24578 )
Change subject: Add diff-scan option to return unobservable rows ...................................................................... Patch Set 2: (3 comments) Overall looks good to me. Just a couple of nits. http://gerrit.cloudera.org:8080/#/c/24578/2/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/24578/2/src/kudu/client/client-test.cc@9568 PS2, Line 9568: TestDiffScanIncludeUnobservableRows nit: Do you think additional variants of the test are required that take into consideration the scans after flushing a) memrowsets and b) delta-mem stores. I understand flushed delta memstore is already taken care in the form of TestUnobservableRowsDiskRowSet but I am not sure if that covers unflushed deltamemstore and just Memrowset to Diskrowset conversion scenario? More specifically this scenario: ++ // Flush the current MemRowSet for this tablet to disk. This swaps // in a new (initially empty) MemRowSet in its place. // // This doesn't flush any DeltaMemStores for any existing RowSets. Status Flush(); ++ IIUC, you can use 'InternalMiniCluster::FlushTablet' to achieve this. This is not mandatory but it wouldn't hurt to add one such scenario just to rule out any surprise in outcome when rows have been flushed from memrowsets to diskrowsets and there are not deltas involved. I am fine with adding that here or in OrderedDiffScanWithDeletesTest test suite. http://gerrit.cloudera.org:8080/#/c/24578/2/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: http://gerrit.cloudera.org:8080/#/c/24578/2/src/kudu/client/scanner-internal.cc@481 PS2, Line 481: scan->set_include_unobservable_rows(configuration_.include_unobservable_rows()); Move this under 'if (configuration_.include_unobservable_rows()) {' check ? http://gerrit.cloudera.org:8080/#/c/24578/2/src/kudu/tablet/diff_scan-test.cc File src/kudu/tablet/diff_scan-test.cc: http://gerrit.cloudera.org:8080/#/c/24578/2/src/kudu/tablet/diff_scan-test.cc@162 PS2, Line 162: bool include_unobservable_rows, : vector<string> *rows) { nit: Fix indentation -- 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: 2 Gerrit-Owner: 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: Tue, 14 Jul 2026 12:57:28 +0000 Gerrit-HasComments: Yes
