Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18029 )
Change subject: [tools] KUDU-3337 Add unsafe_create_cmeta tool ...................................................................... Patch Set 4: (10 comments) http://gerrit.cloudera.org:8080/#/c/18029/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18029/3//COMMIT_MSG@10 PS3, Line 10: There is a : flag to force fsync, but it's disabled by default > That has changed for XFS-based filemanagers with https://github.com/apache/ Done http://gerrit.cloudera.org:8080/#/c/18029/3/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/18029/3/src/kudu/tools/kudu-tool-test.cc@4646 PS3, Line 4646: > nit: add 'const' or maybe even turn this into 'constexr const char* const'? Done http://gerrit.cloudera.org:8080/#/c/18029/3/src/kudu/tools/kudu-tool-test.cc@4651 PS3, Line 4651: er()->tablet_manager()->GetTabletReplicas(&tablet_replicas) > nit: could this be turned into a constant (like 'tablet_id') and reused at Done http://gerrit.cloudera.org:8080/#/c/18029/3/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: http://gerrit.cloudera.org:8080/#/c/18029/3/src/kudu/tools/tool_action_local_replica.cc@665 PS3, Line 665: RETURN_NOT_OK_PREPEND(Env::Default()->RenameFile(original_path, backup_path), : "couldn't back up original file"); > Are we OK with the tool crashing if the passed parameters cannot be parsed Done http://gerrit.cloudera.org:8080/#/c/18029/3/src/kudu/tools/tool_action_local_replica.cc@675 PS3, Line 675: > Is it enough to always have only voter members in the config, or there migh If the tablet in question was a non-voter, it wouldn't affect consensus, and would've already been re-replicated making running this tool pointless, or am I missing something? http://gerrit.cloudera.org:8080/#/c/18029/3/src/kudu/tools/tool_action_local_replica.cc@687 PS3, Line 687: } > I guess having just 'FsManagerOpts fs_opts;' would suffice. Done http://gerrit.cloudera.org:8080/#/c/18029/3/src/kudu/tools/tool_action_local_replica.cc@689 PS3, Line 689: > nit: could be wrapped into std::move() ? Done http://gerrit.cloudera.org:8080/#/c/18029/3/src/kudu/tools/tool_action_local_replica.cc@693 PS3, Line 693: // Parse peer arguments. : for (const auto& arg : context.variadic_args) { > nit: could be reduced? Done http://gerrit.cloudera.org:8080/#/c/18029/3/src/kudu/tools/tool_action_local_replica.cc@1380 PS3, Line 1380: ed tablet's data af > Since this is already in the context of the 'cmeta' sub-command, maybe name Done http://gerrit.cloudera.org:8080/#/c/18029/3/src/kudu/tools/tool_action_local_replica.cc@1388 PS3, Line 1388: "expected and the tablet server works fine with the new state of the tablet's data. " > Does it make sense to add a '--force' flag to rewrite existing cmeta files Done -- To view, visit http://gerrit.cloudera.org:8080/18029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I136cc5b5797420a9ca9156f37c3e281da0c265d7 Gerrit-Change-Number: 18029 Gerrit-PatchSet: 4 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Mon, 06 Nov 2023 09:47:28 +0000 Gerrit-HasComments: Yes
