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 3: (14 comments) http://gerrit.cloudera.org:8080/#/c/24578/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24578/2//COMMIT_MSG@9 PS2, Line 9: A diff scan reports rows whose state changed inside the : (snap_start_timestamp, snap_timestamp] range. Rows live at : snap_start_timestamp and deleted before snap_timestamp are surfaced : via 'include_deleted_rows'; rows inserted or updated inside the range : that are still live at snap_timestamp are surfaced by default. But rows : whose ent > Could you add a bit more color here: in a diff scan's output, how such rows Done 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: > nit: Do you think additional variants of the test are required that take in Added additional tests in OrderedDiffScanWithDeletesTest in diff_scan-test.cc. http://gerrit.cloudera.org:8080/#/c/24578/2/src/kudu/client/client-test.cc@9641 PS2, Line 9641: const shared_ptr<KuduTable>& table, int32_t key) { > Does it make sense to add extra test scenarios: Done http://gerrit.cloudera.org:8080/#/c/24578/2/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/24578/2/src/kudu/client/client.h@3135 PS2, Line 3135: /// @cond PRIVATE_API > Can we also start a patch to make these APIs public? Yes, working on it. http://gerrit.cloudera.org:8080/#/c/24578/2/src/kudu/client/client.h@3171 PS2, Line 3171: the projection to distinguish > nit: consider introducing an enumeration with meaningful field names instea Done http://gerrit.cloudera.org:8080/#/c/24578/2/src/kudu/client/scan_token-internal.cc File src/kudu/client/scan_token-internal.cc: http://gerrit.cloudera.org:8080/#/c/24578/2/src/kudu/client/scan_token-internal.cc@450 PS2, Line 450: pb.set_snap_timestamp(configuration_.snapshot_timestamp()); > nit: consider adding DCHECK() to catch mistakes -- make sure configuration_ Done 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: if (configuration_.include_unobservable_rows()) { > Move this under 'if (configuration_.include_unobservable_rows()) {' Done 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: MvccSnapshot snap1, : MvccSnapshot snap2, > nit: in addition to addressing indentation, make the formatting consistent Done http://gerrit.cloudera.org:8080/#/c/24578/2/src/kudu/tablet/diff_scan-test.cc@162 PS2, Line 162: MvccSnapshot snap1, : MvccSnapshot snap2, > nit: Fix indentation Done http://gerrit.cloudera.org:8080/#/c/24578/2/src/kudu/tablet/diff_scan-test.cc@245 PS2, Line 245: ASSERT_OK(InsertTestRow(&writer, 1, 1)); > Consider adding INSERT after DELETE -- make sure it the row isn't reported Done http://gerrit.cloudera.org:8080/#/c/24578/2/src/kudu/tablet/diff_scan-test.cc@265 PS2, Line 265: MvccSnapshot snap1(*tablet->mvcc_manager()); > Consider adding more of Update/Delete transitions (maybe, in an extra scena Done http://gerrit.cloudera.org:8080/#/c/24578/2/src/kudu/tablet/memrowset.cc File src/kudu/tablet/memrowset.cc: http://gerrit.cloudera.org:8080/#/c/24578/2/src/kudu/tablet/memrowset.cc@716 PS2, Line 716: The row > nit: is set Done http://gerrit.cloudera.org:8080/#/c/24578/2/src/kudu/tablet/rowset.h File src/kudu/tablet/rowset.h: http://gerrit.cloudera.org:8080/#/c/24578/2/src/kudu/tablet/rowset.h@113 PS2, Line 113: ignored otherwise. > Wouldn't it be an inconsistent state? Should there be at least DCHECK to ca Added the needed DCHECKS in memrowset.cc and delta_applier.cc http://gerrit.cloudera.org:8080/#/c/24578/2/src/kudu/tserver/tserver.proto File src/kudu/tserver/tserver.proto: http://gerrit.cloudera.org:8080/#/c/24578/2/src/kudu/tserver/tserver.proto@364 PS2, Line 364: > Is it possible to make this more certain? In other words, does the server Yes, added a clearer comment. -- 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: 3 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: Wed, 22 Jul 2026 00:16:23 +0000 Gerrit-HasComments: Yes
