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 9: (9 comments) http://gerrit.cloudera.org:8080/#/c/24578/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24578/9//COMMIT_MSG@49 PS9, Line 49: bool > enum? Done http://gerrit.cloudera.org:8080/#/c/24578/9/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/24578/9/src/kudu/client/client-test.cc@9688 PS9, Line 9688: ASSERT_FALSE(status.is_deleted); > Shouldn't this be TRUE? Yes, thanks, done http://gerrit.cloudera.org:8080/#/c/24578/9/src/kudu/client/client-test.cc@9727 PS9, Line 9727: ASSERT_FALSE(status.is_deleted); > ditto Done http://gerrit.cloudera.org:8080/#/c/24578/9/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/24578/9/src/kudu/client/client.h@3154 PS9, Line 3154: DiffScanRowVisibility > nit: Should this have 1-to-1 mapping with RowVisibility with addition of de Done http://gerrit.cloudera.org:8080/#/c/24578/9/src/kudu/client/scan_token-internal.cc File src/kudu/client/scan_token-internal.cc: http://gerrit.cloudera.org:8080/#/c/24578/9/src/kudu/client/scan_token-internal.cc@301 PS9, Line 301: const auto visibility = message.row_visibility() == kudu::INCLUDE_UNOBSERVABLE : ? KuduScanner::INCLUDE_UNOBSERVABLE : : KuduScanner::OBSERVABLE_ONLY; > q: Is 'visibility' intentionally set to OBSERVABLE_ONLY if message's row_vi Yes, mapping UNKNOWN_ROW_VISIBILITY to OBSERVABLE_ONLY here is intentional. The switch runs on decoded ScanTokenPB bytes and if an older client serializes the token where the field doesn't exist in its proto and the reader gets zero value. http://gerrit.cloudera.org:8080/#/c/24578/9/src/kudu/client/scan_token-internal.cc@458 PS9, Line 458: DCHECK(configuration_.has_start_timestamp() && : configuration_.has_snapshot_timestamp()) : << "row_visibility=INCLUDE_UNOBSERVABLE requires a diff scan " : "(both start and snapshot timestamps must be set)"; > Should this be a CHECK instead, to align with similar CHECK inside ScanConf Done http://gerrit.cloudera.org:8080/#/c/24578/9/src/kudu/tablet/delta_applier.cc File src/kudu/tablet/delta_applier.cc: http://gerrit.cloudera.org:8080/#/c/24578/9/src/kudu/tablet/delta_applier.cc@30 PS9, Line 30: #include "kudu/common/common.pb.h" > why is this included here along with no_include pragma below for the same h Ah, this was an error. Removed this line 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 Done 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)"); : } > +1: it would be nice to have very basic test scenarios that show how the se Done -- 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: Thu, 20 Aug 2026 05:34:22 +0000 Gerrit-HasComments: Yes
