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 2: (10 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 currently drops rows whose entire lifecycle (insert -> : eventual delete) is contained inside the (start_ts, end_ts] range. But : if a user wants to fetch these rows i.e. if they need every : mutation the tablet saw during the range -- e.g. change-data-capture : pipelines that reconstruct row-level history, this patch makes it : possible. Could you add a bit more color here: in a diff scan's output, how such rows differ from rows that appeared/inserted before start_ts, but were deleted before end_ts (depending whether 'include_deleted_rows' is set false/true)? 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@9641 PS2, Line 9641: Does it make sense to add extra test scenarios: 1) insert, delete, and then insert the row again; make sure the row is present as a regular row and isn't reported as deleted via row.IsDeleted() 2) insert, delete, and then insert and delete the row again; make sure the row isn't present, but reported as deleted via row.IsDeleted() 3) add variations: use UPSERT instead of INSERT 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@3171 PS2, Line 3171: bool include_unobservable_rows nit: consider introducing an enumeration with meaningful field names instead of using the bool type here. 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: if (configuration_.include_unobservable_rows()) { nit: consider adding DCHECK() to catch mistakes -- make sure configuration_.include_unobservable_rows() is set true only when configuration_.has_snapshot_timestamp() && configuration_.has_start_timestamp() 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 nit: in addition to addressing indentation, make the formatting consistent and place each parameter into its own line http://gerrit.cloudera.org:8080/#/c/24578/2/src/kudu/tablet/diff_scan-test.cc@245 PS2, Line 245: ASSERT_OK(DeleteTestRow(&writer, 1)); Consider adding INSERT after DELETE -- make sure it the row isn't reported as deleted (maybe, do so in a separate scenario) http://gerrit.cloudera.org:8080/#/c/24578/2/src/kudu/tablet/diff_scan-test.cc@265 PS2, Line 265: ASSERT_OK(InsertTestRow(&writer, 1, 1)); Consider adding more of Update/Delete transitions (maybe, in an extra scenario). 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: ends up nit: is set 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 catch such conditions when include_unobservable_rows=true and iterator isn't configured for a diff scan? 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: Ignored (or rejected) Is it possible to make this more certain? In other words, does the server side spews an error on such condition, or silently ignores the 'include_unobservable_rows' field if it's set, or it's impossible to stick to any particular policy for some reason? -- 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: Wed, 15 Jul 2026 19:39:11 +0000 Gerrit-HasComments: Yes
