Yingchun Lai 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 7: (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: 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. > There is 'kudu pbc edit' CLI tool to achieve that, but it's error prone and Done http://gerrit.cloudera.org:8080/#/c/19357/6//COMMIT_MSG@19 PS6, Line 19: a > a new Done http://gerrit.cloudera.org:8080/#/c/19357/6//COMMIT_MSG@20 PS6, Line 20: which makes removing rowsets from a tablet much easier. > which makes removing rowsets from a tablet much easier Done 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: > Instead of another tricky flag that might corrupt the metadata (in the sens It's not enough safe and convenient if just disable --enable_tablet_orphaned_block_deletion. Without the new added --enable_tablet_orphaned_block_adding, after blocks have been added to 'orphaned_blocks_', they will be persisted to super block PB file. So we have to change --enable_tablet_orphaned_block_deletion to false (which is not the default value) when use the CLI tool and the next time when restart server to check if the operation is expected. If some one forget to chenge the default value when restart the server, blocks will be deleted and the backup file is useless then. 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 Agree, done. http://gerrit.cloudera.org:8080/#/c/19357/6/src/kudu/tools/kudu-tool-test.cc@8844 PS6, Line 8844: metadata_pa > nit: original_file Done http://gerrit.cloudera.org:8080/#/c/19357/6/src/kudu/tools/kudu-tool-test.cc@8898 PS6, Line 8898: LSE(backup_f > nit: 'no data is lost' or 'there isn't any data loss' Done 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 Done http://gerrit.cloudera.org:8080/#/c/19357/6/src/kudu/tools/tool_action_local_replica.cc@640 PS6, Line 640: if (FLAGS_backup_metadata) { : // Move the old tablet metadata file to a backup location. : string original_path = fs_manager.GetTabletMetadataPath(tablet_id); : string backup_path = Substitute("$0.bak.$1", original_path, GetCurrentTimeMicros()); : RETURN_NOT_OK_PREPEND(Env::Default()->RenameFile(original_path, backup_path), : "couldn't back up original file"); : LOG(INFO) << "Moved original file to " << backup_path; : } : > Maybe, move this input verification to be after other checks at lines 622-6 Good idea. http://gerrit.cloudera.org:8080/#/c/19357/6/src/kudu/tools/tool_action_local_replica.cc@662 PS6, Line 662: s::InvalidArgument("no tablet > nit: Successfully removed rowsets with identifiers: Done http://gerrit.cloudera.org:8080/#/c/19357/6/src/kudu/tools/tool_action_local_replica.cc@1276 PS6, Line 1276: ionalParameter("enable_tablet_orpha > I'd suggest to use already existing --enable_tablet_orphaned_block_deletion Emm, OK, add a more tricky flag maybe not reasonable enough, Idecide to remove the new added flag after trade-off the two choices. And it's able to enable --backup_metadata by default, which Yuqi suggest. When administrators input some incorrect rowset ids, it works not as expected. http://gerrit.cloudera.org:8080/#/c/19357/6/src/kudu/tools/tool_action_local_replica.cc@1275 PS6, Line 1275: : .AddOptionalParameter("enable_tablet_orphaned_block_deletion") : .AddOptionalParameter("fs_data_dirs") : .AddOptionalParameter("fs_metadata_dir") : .AddOptionalParameter("fs_wal_dir") : .Build(); : > Move this into ExtraDescription(). See https://github.com/apache/kudu/blob Done -- 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: 7 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 16:57:19 +0000 Gerrit-HasComments: Yes
