Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7853 )
Change subject: tool: add cluster shell action ...................................................................... Patch Set 8: (11 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 This should use an underscore to be consistent with the rest of our libraries. 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. What's the usecase for having multiple clusters? Can't a client just spawn multiple instances if they need multiple clusters? http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto@29 PS8, Line 29: required int32 cluster_id = 1; use optional http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto@61 PS8, Line 61: required int32 cluster_id = 1; use optional http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto@71 PS8, Line 71: required int32 cluster_id = 1; optional http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool.proto@80 PS8, Line 80: required int32 cluster_id = 1; optional, etc. 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 a field, do bigger reads, and use something like strnchr to find the newline. 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. http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@718 PS8, Line 718: CHECK_EQ(buf->length(), r); This isn't guaranteed, per man 2 read: > Upon successful completion, read(), readv(), and pread() return the number of > bytes actually read and placed in the buffer. The system guarantees to read > the number of bytes requested if the descriptor references a normal file that > has that many bytes left before the end-of-file, but in no other case. I think you may see this in practice if the client sends the length header, and then sends the request body split across multiple write() calls (perhaps pausing in between to make it more racy). 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@216 PS8, Line 216: opts.master_rpc_ports = { 11030, 11031, 11032 }; > nit: add DCHECK_EQ(3, opts.num_masters) ? It's checked above on line 196. Isn't the hard-coded master ports going to cause problems for multi-master, multi-cluster shells? I think this is another good reason not to support multiple clusters. http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_test.cc@346 PS8, Line 346: ActionBuilder("shell", &RunControlShell) I'm opposed to calling this a shell. There is precedent for the term shell with noSQL databases, and it's not this. People regularly ask if there's a Kudu shell, and they aren't asking for this. Eventually we might have a proper shell in the kudu tool. I think 'kudu mini_cluster' is a better name, given that mini-cluster is an established term of art. -- 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: Tue, 03 Oct 2017 15:24:47 +0000 Gerrit-HasComments: Yes
