Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7853 )
Change subject: tool: new action for running mini-clusters ...................................................................... Patch Set 8: (4 comments) http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto File src/kudu/tools/tool.proto: http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto@28 PS8, Line 28: // The ID of the cluster, unique to the control shell. > Once you've gone that far, it's trivial to convert that > optional<ExternalMiniCluster> into a map<int, ExternalMiniCluster> and > support multiple clusters, so I did it even though there's no explicit use > case. FWIW, think reusing a single control instance for that is preferable > than forcing clients to manage multiple instances. And it's trivially easy for the user to manage this on their end through a similar map of ID -> MiniCluster. I don't see the advantage of building it into the protocol when there's a perfectly fine workaround, and we don't know of any potential uses. http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto@29 PS8, Line 29: required int32 cluster_id = 1; > Why? This message is useless if it doesn't include the cluster_id. required fields should never be added to protobuf messages. This isn't specific to mini-cluster, it's just a protobuf best practice. http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto@97 PS8, Line 97: message StopProcessRequestPB { Would this API be used for HMS/Sentry processes if/when those are added to the mini cluster? http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc File src/kudu/tools/tool_action_test.cc: http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc@346 PS8, Line 346: ActionBuilder("shell", &RunControlShell) > I'll change it to "mini_cluster", but I'm still going to also refer to it a SGTM, i'd just suggest fully qualifying it as 'mini-cluster control shell' wherever it might be ambiguous. -- To view, visit http://gerrit.cloudera.org:8080/7853 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0e693921ef780dc4a06e536c6b7408f7f0b252f6 Gerrit-Change-Number: 7853 Gerrit-PatchSet: 8 Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 04 Oct 2017 19:29:55 +0000 Gerrit-HasComments: Yes
