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 7: (7 comments) 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: The RPC field is a new 'optional bool' and is wire-compatible with : older servers (which ignore it) and older clients (which don't set it). How is it compatible with older servers when they simply cannot interpret the request properly, and return data inconsistent with the request? This isn't that can be called compatible, not at all. Shouldn't there be a tablet server feature flag introduced to spot this incompatibility? http://gerrit.cloudera.org:8080/#/c/24578/7/src/kudu/client/client.proto File src/kudu/client/client.proto: http://gerrit.cloudera.org:8080/#/c/24578/7/src/kudu/client/client.proto@101 PS7, Line 101: Unknown = 0; Should the newly added field should come with a new feature flag? In other words, can clients that aren't aware of the visibility of ephemeral rows interpret an instance of ScanTokenPB message wrongly or legacy clients can properly process/interpret tokens with include_unobservable_rows field set? http://gerrit.cloudera.org:8080/#/c/24578/7/src/kudu/client/client.proto@169 PS7, Line 169: bool include_unobservable_rows = 26 Consider introducing an enumeration here, similar to OrderMode and/or ReadMode. In addition to more explicit and easier to read code, it also future-proof in the sense to extending this field in future, if a new mode of treating ephemeral rows is ever introduced later on. http://gerrit.cloudera.org:8080/#/c/24578/7/src/kudu/client/scan_configuration.h File src/kudu/client/scan_configuration.h: http://gerrit.cloudera.org:8080/#/c/24578/7/src/kudu/client/scan_configuration.h@94 PS7, Line 94: bool include_unobservable_rows = false Any particular reason of not using DiffScanRowVisibility here as well? If it seems more comfortable using boolean in the signature of the include_unobservable_rows() accessor, one option might be keeping the visibility represented via DiffScanRowVisibility enum internally, while the accessor can still return boolean type, comparing the internally stored value with KuduScanner::INCLUDE_UNOBSERVABLE What do you think? http://gerrit.cloudera.org:8080/#/c/24578/7/src/kudu/client/scan_configuration.h@230 PS7, Line 230: bool include_unobservable_rows_; Would it make more sense to store KuduScanner::DiffScanRowVisibility here instead? http://gerrit.cloudera.org:8080/#/c/24578/7/src/kudu/tablet/rowset.h File src/kudu/tablet/rowset.h: http://gerrit.cloudera.org:8080/#/c/24578/7/src/kudu/tablet/rowset.h@116 PS7, Line 116: include_unobservable_rows Would it make sense to store KuduScanner::DiffScanRowVisibility here insted? It's quite similar to OrderMode -- we store OrderMode instead of boolean, and the values for the field can be UNORDERED or ORDERED. http://gerrit.cloudera.org:8080/#/c/24578/7/src/kudu/tserver/tserver.proto File src/kudu/tserver/tserver.proto: http://gerrit.cloudera.org:8080/#/c/24578/7/src/kudu/tserver/tserver.proto@361 PS7, Line 361: // When set on a diff scan, rows whose entire lifecycle lies inside : // (snap_start_timestamp, snap_timestamp] i.e. rows that were inserted : // and deleted within the range will still be returned (marked deleted via : // the IS_DELETED virtual column). : // : // Only valid on diff scans. The server returns INVALID_SCAN_SPEC if this : // field is set without snap_start_timestamp, or on a diff scan whose : // projection lacks an IS_DELETED virtual column. : optional bool include_unobservable_rows = 17; Shouldn't this update come along with a new feature flag added into TabletServerFeatures, so a Kudu client that sets visibility for ephemeral rows in diffscan know whether the target server is able to process scan request with this field set? -- 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: 7 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-Comment-Date: Mon, 10 Aug 2026 17:27:42 +0000 Gerrit-HasComments: Yes
