Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19357 )
Change subject: [tools] Add 'kudu local_replica tmeta delete_rowsets' to delete rowsets from tablet ...................................................................... Patch Set 6: (12 comments) http://gerrit.cloudera.org:8080/#/c/19357/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19357/6//COMMIT_MSG@16 PS6, Line 16: Although we can use 'kudu pbc edit' to achieve this aim, but it's very hard to : operate when the tablet-metadata is dozens of megabytes. There is 'kudu pbc edit' CLI tool to achieve that, but it's error prone and hard to operate when working with large amount of data. http://gerrit.cloudera.org:8080/#/c/19357/6//COMMIT_MSG@19 PS6, Line 19: a a new http://gerrit.cloudera.org:8080/#/c/19357/6//COMMIT_MSG@20 PS6, Line 20: to delete rowsets from a tablet, which is easier to use which makes removing rowsets from a tablet much easier http://gerrit.cloudera.org:8080/#/c/19357/6/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: http://gerrit.cloudera.org:8080/#/c/19357/6/src/kudu/tablet/tablet_metadata.cc@68 PS6, Line 68: enable_tablet_orphaned_block_adding Instead of another tricky flag that might corrupt the metadata (in the sense of not tracking the blocks to be deleted), maybe use already existing flag --enable_tablet_orphaned_block_deletion? http://gerrit.cloudera.org:8080/#/c/19357/6/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/19357/6/src/kudu/tools/kudu-tool-test.cc@a6028 PS6, Line 6028: nit: why is this change? I'd suggest making changes that are relevant only to the essence of the patch, otherwise this sort of a change might result in an extra conflict when trying to cherry-pick this. http://gerrit.cloudera.org:8080/#/c/19357/6/src/kudu/tools/kudu-tool-test.cc@8844 PS6, Line 8844: origin_file nit: original_file http://gerrit.cloudera.org:8080/#/c/19357/6/src/kudu/tools/kudu-tool-test.cc@8898 PS6, Line 8898: no data lost nit: 'no data is lost' or 'there isn't any data loss' http://gerrit.cloudera.org:8080/#/c/19357/6/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: http://gerrit.cloudera.org:8080/#/c/19357/6/src/kudu/tools/tool_action_local_replica.cc@106 PS6, Line 106: edit editing http://gerrit.cloudera.org:8080/#/c/19357/6/src/kudu/tools/tool_action_local_replica.cc@640 PS6, Line 640: RowSetMetadataIds to_remove; : for (const auto& rowset_id_str : rowset_ids_vec) { : int64_t rowset_id; : if (safe_strto64(rowset_id_str.c_str(), &rowset_id)) { : to_remove.insert(rowset_id); : } else { : return Status::InvalidArgument(Substitute("$0 is not a valid rowset id.", rowset_id_str)); : } : } Maybe, move this input verification to be after other checks at lines 622-627 and before opening FSManager? http://gerrit.cloudera.org:8080/#/c/19357/6/src/kudu/tools/tool_action_local_replica.cc@662 PS6, Line 662: Succeed to remove rowset ids: nit: Successfully removed rowsets with identifiers: http://gerrit.cloudera.org:8080/#/c/19357/6/src/kudu/tools/tool_action_local_replica.cc@1276 PS6, Line 1276: enable_tablet_orphaned_block_adding I'd suggest to use already existing --enable_tablet_orphaned_block_deletion flag here for both running the tool and then for a dry-run of kudu-tserver to verify that the operation worked as expected. BTW, what are conditions when the 'delete_rowsets' works not as expected? http://gerrit.cloudera.org:8080/#/c/19357/6/src/kudu/tools/tool_action_local_replica.cc@1275 PS6, Line 1275: You can set " : "--enable_tablet_orphaned_block_adding to false, then only " : "rowset ids are removed from tablet metadata, the related blocks " : "are kept on file system. You can start the server to check " : "whether the operation is worked as expect, if it is, then " : "shutdown the server and run 'kudu fs check --repair' to remove " : "orphaned blocks, or use the backup tablet-metadata to roll back.") Move this into ExtraDescription(). See https://github.com/apache/kudu/blob/986f1a0cd0427e548a8bff7ccc53b626335ebc57/src/kudu/tools/tool_action_remote_replica.cc#L473-L479 for an example. -- To view, visit http://gerrit.cloudera.org:8080/19357 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If2cf9035babf4c3af4c238cebe8dcecd2c65848f Gerrit-Change-Number: 19357 Gerrit-PatchSet: 6 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Wed, 04 Jan 2023 02:39:30 +0000 Gerrit-HasComments: Yes
