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 12: (8 comments) http://gerrit.cloudera.org:8080/#/c/24578/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24578/12//COMMIT_MSG@30 PS12, Line 30: include_unobservable_rows here and elsewhere: 'include_unobservable_rows' isn't present in NewScanRequestPB as of PS12 http://gerrit.cloudera.org:8080/#/c/24578/12/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/24578/12/src/kudu/client/client.h@3150 PS12, Line 3150: Status SetDiffScan(uint64_t start_timestamp, uint64_t end_timestamp) : WARN_UNUSED_RESULT KUDU_NO_EXPORT; After revisiting changes in PS12, I just realized this flavor of the SetDiffScan method hasn't been exported, so it's not a part of the public ABI for libkudu_client. Given it's not exported in this changelist, but the new flavor of the method with signature Status SetDiffScan(uint64_t, uint64_t, DiffScanRowVisibility) is now exported, should we remove this non-exported flavor and change all the existing call sites to switch to the new flavor? Same for the KuduScanTokenBuilder's counterparts. Essentially, the absence of SetDiffScan() in the so-far-published ABI gives us a wider range of options here, and we are free to change the signature of the function because SetDiffScan() couldn't be seen in the earlier versions of the exported library. https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B What do you think? http://gerrit.cloudera.org:8080/#/c/24578/12/src/kudu/client/client.proto File src/kudu/client/client.proto: http://gerrit.cloudera.org:8080/#/c/24578/12/src/kudu/client/client.proto@172 PS12, Line 172: // See kudu.tserver.NewScanRequestPB.row_visibility. When set to : // INCLUDE_UNOBSERVABLE, the emitting client must also add : // ScanTokenPB.Feature::RowVisibility to feature_flags so that a : // deserializing client without this field's semantics fails cleanly. : optional RowVisibility row_visibility = 26 [default = OBSERVABLE_ONLY]; It would be nice to cover how this new field goes through serialization --> deserialization cycle, so the result scanner indeed have the proper field set in the configuration. Maybe, add something similar to verification of scan timeout in the ScanTokenTest.TestScanTokens scenario? Along with that, it should be great to check for the value of the 'feature_flags' field depending on the 'row_visibility' setting. http://gerrit.cloudera.org:8080/#/c/24578/12/src/kudu/client/scan_configuration.cc File src/kudu/client/scan_configuration.cc: http://gerrit.cloudera.org:8080/#/c/24578/12/src/kudu/client/scan_configuration.cc@65 PS12, Line 65: row_format_flags_(KuduScanner::NO_FLAGS), : row_visibility_(KuduScanner::OBSERVABLE_ONLY) { Aren't there reversed compared with their declaration order in the header file? http://gerrit.cloudera.org:8080/#/c/24578/12/src/kudu/tablet/diff_scan-test.cc File src/kudu/tablet/diff_scan-test.cc: http://gerrit.cloudera.org:8080/#/c/24578/12/src/kudu/tablet/diff_scan-test.cc@174 PS12, Line 174: opts.snap_to_include = snap_to_include; nit: add std::move() to avoid extra copying? http://gerrit.cloudera.org:8080/#/c/24578/12/src/kudu/tablet/diff_scan-test.cc@175 PS12, Line 175: opts.order = ORDERED; IIUC, in the new test scenarios the only covered ordering option is ORDERED when opts.row_visibility is set to INCLUDE_UNOBSERVABLE. Is there any interest in covering the UNORDERED option, at least to be explicit whether a tablet server is supposed to respond with an error in such a case. If tablet server isn't supposed to response with an error, maybe add something very basic, similar to DiffScanTest.DiffScan? Essentially, that's to cover the functionality of the union scan iterator for the new visibility option in case of UNORDERED row iterator option. http://gerrit.cloudera.org:8080/#/c/24578/12/src/kudu/tablet/diff_scan-test.cc@483 PS12, Line 483: SCOPED_TRACE(rv == INCLUDE_UNOBSERVABLE ? "INCLUDE_UNOBSERVABLE" : : "OBSERVABLE_ONLY"); Here and elsewhere: is it possible to use the auto-generated RowVisibility_Name() from common.pb.h instead? http://gerrit.cloudera.org:8080/#/c/24578/12/src/kudu/tablet/diff_scan-test.cc@520 PS12, Line 520: These newly added synthetic test scenarios always use a single row key. Does it make sense to add a test scenario or two where we'd have a mix of a few rows, where different rows would be of different lifecycle regarding their INSERT, UPDATE, and DELETE times regarding the target (from, to] scan interval? Something like below: key0: non-existent at snap_start, then INSERT/UPSERT, DELETE before snap_end key1: non-existent at snap_start, then INSERT/UPSERT, DELETE, INSERT/UPSERT before snap_end key2: already present at snap_start, then DELETE before snap_end key3: already present at snap_start, no changes before snap_end -- 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: 12 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: Tue, 25 Aug 2026 01:58:32 +0000 Gerrit-HasComments: Yes
