Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/7853 )
Change subject: tool: new action for running mini-clusters ...................................................................... Patch Set 8: (27 comments) http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/CMakeLists.txt File src/kudu/tools/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/CMakeLists.txt@120 PS8, Line 120: mini-cluster > nevermind, bad suggestion Heh, between the time you made the suggestion and the time you rescinded it I wrote a patch to change mini-cluster to mini_cluster (and integration-tests to itest_util). So, too late. http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/kudu-tool-test.cc@1843 PS8, Line 1843: ASSERT_FALSE(s.ok()); : ASSERT_TRUE(s.IsInvalidArgument()); > here and below: why is it necessary to verify for ASSERT_FALSE(s.ok()); if I instinctively did it this way, but I have no good explanation for why. I'll remove it. 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. > Taking this a step further, I think the API would be better/simpler if the My original prototype used gflags for cluster options. Once I transitioned to this structured control protocol, I didn't think it made sense to do that anymore, because that meant marshaling some stuff via PB/json (commands) and the rest through gflags. For trivial options it doesn't matter, but for more complicated ones (like extra_tserver_flags), I'd prefer to marshal everything the same way. In the future I imagine we'll expose even more options and I'd much rather do so via PB/json than figuring out how to shoehorn them into gflags. Alexey also suggested this approach, I believe. Given the above, we have to bring up the shell "empty" and force clients to explicitly create a cluster. But we also want to support "cluster stop" separately from "cluster stop and destroy", because some tests might want to restart their cluster. So that means the shell needs to be at least stateful enough to differentiate between "cluster exists" and "cluster does not exist". 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. http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto@29 PS8, Line 29: required int32 cluster_id = 1; > use optional Why? This message is useless if it doesn't include the cluster_id. http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto@60 PS8, Line 60: The generated ID of the cluster, unique to the control shell. > maybe just: 'ID of the cluster to destroy.' ? Done http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto@61 PS8, Line 61: required int32 cluster_id = 1; > use optional See above. http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto@71 PS8, Line 71: required int32 cluster_id = 1; > optional See above. http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto@80 PS8, Line 80: required int32 cluster_id = 1; > optional, etc. See above. http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto@91 PS8, Line 91: required HostPortPB process_address > How does it work for multiple addresses (corresponding to '--rpc_bind_addre Minicluster daemons are only ever bound to one address, and that address (with the port) is always unique and can be used to identify the daemon. Will we ever bind daemons to multiple addresses? Possibly, but it's tough to anticipate exactly what that would look like. Commands that identify a daemon via address could still work because every address is still unique, but Get{Master,Tserver}Addresses may have to change. http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto@104 PS8, Line 104: HostPortPB > Same question if multiple addresses are allowed: which address should be sp See above. http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc File src/kudu/tools/tool_action_common.cc: http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@613 PS8, Line 613: // Read and accumulate one byte at a time, looking for the newline. > Seems like it would be easier and more efficient to keep the read buffer as It would be, but I didn't think it really mattered since this control protocol (and JSON mode) is seldom used, and it means maintaining real state (i.e. a partially read buffer). I'll add a TODO though. http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@616 PS8, Line 616: faststring one_byte; > hoist this out of the loop. Done http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@663 PS8, Line 663: LOG(INFO) > nit: looks like more VLOG(1) to me. Done http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@669 PS8, Line 669: LOG(INFO) > ditto: VLOG(1) ? Done http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@680 PS8, Line 680: unable to print JSON to stdout > nit: 'unable to serialize into JSON: $0'? Done http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@684 PS8, Line 684: buf.append(serialized); > Have you checked what happens if the message has a string field containing It looks like they're properly escaped. I tested by adding a "\n" to a cluster's data_root option and the protocol still worked. http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@685 PS8, Line 685: buf.append("\n"); > Ah, I see. For some reason I thought the serialized JSON is also preceded Correct; see the SerializationMode class comment. http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@718 PS8, Line 718: CHECK_EQ(buf->length(), r); > Ah, I played with that and I can see you are right: it's possible for pipes Hmm, OK, I guess I'll make this resilient to short reads. http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@735 PS8, Line 735: CHECK_EQ(buf.length(), r); > Yep, as I found, short reads are possible for pipes, indeed (shame on me: I Done 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@20 PS8, Line 20: <stdlib.h> > nit: please use <cstdlib> instead. Done http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc@64 PS8, Line 64: ; > nit: remove extra semicolon Done http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc@102 PS8, Line 102: InvalidArgument > nit: maybe, NotFound is the better choice here? Done http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc@133 PS8, Line 133: InvalidArgument > ditto regarding NotFound Done http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc@150 PS8, Line 150: FLAGS_serialization == "json" > As I see, case-insensitive comparison (i.e. boost::iequals(v, "json")) is u Done http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc@153 PS8, Line 153: "pb", FLAGS_serialization > ditto for case-insensitive comparison. Done http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc@216 PS8, Line 216: opts.master_rpc_ports = { 11030, 11031, 11032 }; > Yes, num_master can only be 3 at this point if the code at line 196 stands Yes, the hard-coding means multiple clusters won't work right now with multiple shells, but that's something we've discussed fixing (i.e. by binding to ephemeral ports when creating the cluster, then passing those bound sockets into the processes themselves). http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc@346 PS8, Line 346: ActionBuilder("shell", &RunControlShell) > I also think 'kudu test mini_cluster' is a good option. I'm not advocating I'll change it to "mini_cluster", but I'm still going to also refer to it as a "control shell" in documentation. I think it's important, otherwise people might get the impression that just running the tool will cause something to happen rather than drop them into a shell. -- 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: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 04 Oct 2017 19:08:20 +0000 Gerrit-HasComments: Yes
