Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13937 )
Change subject: [ksck] Filter tables and tablets in KsckCluster ...................................................................... Patch Set 7: (2 comments) http://gerrit.cloudera.org:8080/#/c/13937/7/src/kudu/tools/ksck.h File src/kudu/tools/ksck.h: http://gerrit.cloudera.org:8080/#/c/13937/7/src/kudu/tools/ksck.h@452 PS7, Line 452: // Setters for filtering the tables/tablets to be checked. : // : // Filter strings are glob-style patterns. For example, 'Foo*' matches : // all tables whose name begins with 'Foo'. : // : // If tables is not empty, checks only the named tables. : // If tablets is not empty, checks only the specified tablet IDs. : // If both are specified, takes the intersection. : // If both are empty (unset), all tables and tablets are checked. Rather than duplicating all of this, could you just refer to the equivalent Ksck functionality? Same for set_tablet_id_filters(). http://gerrit.cloudera.org:8080/#/c/13937/5/src/kudu/tools/ksck_checksum.cc File src/kudu/tools/ksck_checksum.cc: http://gerrit.cloudera.org:8080/#/c/13937/5/src/kudu/tools/ksck_checksum.cc@502 PS5, Line 502: msg += " Filter:"; > It's for passing the test`TestNonMatchingTabletIdFilter`. Yeah I'm not sure I understand either; it seems like num_replicas_tmp should have included both filtered and unfiltered replicas for the "all range partitions were dropped" case to be represented correctly. In any case, it'd be good to achieve the semantics described by the comment. Maybe we could do that by tracking the number of replicas we've filtered out for each table? Then we could do the following check here: if (num_tablets > 0 && (num_replicas_tmp + num_replicas_filtered) == 0) { <no tablet replicas error> } Also could you add a unit test to provide coverage for this case? Both "all range partitions dropped" and "all but one range partitions dropped and that last range partition filtered out". -- To view, visit http://gerrit.cloudera.org:8080/13937 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I23b6e6ef258d3498a42af7f92b63392a59c99761 Gerrit-Change-Number: 13937 Gerrit-PatchSet: 7 Gerrit-Owner: Yifan Zhang <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Fri, 02 Aug 2019 17:03:59 +0000 Gerrit-HasComments: Yes
