Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/7853 )
Change subject: tool: add cluster shell action ...................................................................... Patch Set 8: (4 comments) 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@685 PS8, Line 685: buf.append("\n"); > It's necessary; the format is newline delimited JSON messages. Ah, I see. For some reason I thought the serialized JSON is also preceded with length of the block, but it seems it's not and '\n' is used as a delimiter. http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@718 PS8, Line 718: CHECK_EQ(buf->length(), r); > Correct, the issue is a short read, not a zero read. I'm not sure that's possible for pipes. From 'man 7 pipe': If a process attempts to read from an empty pipe, then read(2) will block until data is available. If a process attempts to write to a full pipe (see below), then write(2) blocks until sufficient data has been read from the pipe to allow the write to complete. Do you have an example where short reads are visible for pipes? Just curious. http://gerrit.cloudera.org:8080/#/c/7853/8/src/kudu/tools/tool_action_common.cc@735 PS8, Line 735: CHECK_EQ(buf.length(), r); > Short writes are also possible. I agree that in general short reads are possible, but I'm not sure that's possible for pipes. As for writes, I don't think short writes are possible at all (unless there is an error, interrupt by signal handler, or the descriptor is set to work in non-blocking mode). Could you point to some reference/example where it's clear to see that 'short' write is possible in blocking mode? I found some info at http://www.linux-mag.com/id/308/ (which is Linux specific and might be obsolete now), but even from there I could not see write might return earlier in non-blocking mode. 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'm in favor of mini_cluster because it's discoverable, self-describing, an I also think 'kudu test mini_cluster' is a good option. I'm not advocating for 'control' over 'mini_cluster', just throwing in some more options to choose from. -- 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 <a...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Tue, 03 Oct 2017 22:04:47 +0000 Gerrit-HasComments: Yes
