Alexey Serbin 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 9: (4 comments) Thank you for revving the patch! Overall looks OK, but please address feedback from other reviewers and consider extending test coverage for invalid/incorrect input from the client side. Also, it would be great to make sure that we are getting expected results even with IGNORE flavor of the DML operations, if it makes sense. Also, please rebase this patch to include recent updates on the 3rd-party build -- it should help avoiding polluting the CI workspace with non-compatible layout of 3rd-party components. Also, dist-test should start passing after the rebase. http://gerrit.cloudera.org:8080/#/c/24578/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24578/7//COMMIT_MSG@40 PS7, Line 40: IS_DELETED virtual column. : > The needed feature flag (TabletServerFeatures::DIFF_SCAN_ROW_VISIBILITY) ha Thank you! http://gerrit.cloudera.org:8080/#/c/24578/9/src/kudu/tablet/diff_scan-test.cc File src/kudu/tablet/diff_scan-test.cc: http://gerrit.cloudera.org:8080/#/c/24578/9/src/kudu/tablet/diff_scan-test.cc@168 PS9, Line 168: bool include_unobservable_rows, Would it make sense to switch to INCLUDE_UNOBSERVABLE/OBSERVABLE_ONLY enum here as well instead of using bool? http://gerrit.cloudera.org:8080/#/c/24578/9/src/kudu/tablet/diff_scan-test.cc@439 PS9, Line 439: In the newly added scenarios above, does it make sense to test for the IGNORE flavor of DML operations as well (e.g., use InsertIgnoreTestRows())? Also, what's the result for the following sequence of operations on the same primary key: INSERT DELETE DELETE_IGNORE INSERT ? It would be great clarify where the row is reported as deleted when INCLUDE_UNOBSERVABLE is set. Also, one more interesting case: INSERT <-- ts_begin INSERT_IGNORE DELETE <-- ts_end For the (ts_begin, ts_end) range, would there be a difference between diffscan results in INCLUDE_UNOBSERVABLE and OBSERVABLE_ONLY modes? http://gerrit.cloudera.org:8080/#/c/24578/9/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/24578/9/src/kudu/tserver/tablet_service.cc@3033 PS9, Line 3033: // Reject row_visibility=INCLUDE_UNOBSERVABLE unless this is a diff scan : // (i.e. snap_start_timestamp is set). Enforcing this uniformly here : // covers every read mode and avoids any silent-ignore path further down. : if (PREDICT_FALSE(scan_pb.row_visibility() == INCLUDE_UNOBSERVABLE && : !scan_pb.has_snap_start_timestamp())) { : *error_code = TabletServerErrorPB::INVALID_SCAN_SPEC; : return Status::InvalidArgument( : "row_visibility=INCLUDE_UNOBSERVABLE may only be set on a diff scan " : "(requires snap_start_timestamp)"); : } > Here and line 3390-3396: Should these new rejection conditions have corresp +1: it would be nice to have very basic test scenarios that show how the server side handles invalid input from the client side. That should help us: 1) be explicit about how server side handles such situations 2) avoid regressions if anything is changed in this code later 3) make sure that invalid client input doesn't crash server (at least in RELEASE node, in DEBUG we might use DCHECK() to spot issues early). -- 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: 9 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: Wed, 19 Aug 2026 06:14:40 +0000 Gerrit-HasComments: Yes
