Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15574 )
Change subject: subprocess: use a fifo instead of stdout for IO ...................................................................... Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/15574/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15574/2//COMMIT_MSG@7 PS2, Line 7: subprocess: use a fifo instead of stdout for IO > What about stdin? Wouldn't it be more intuitive if all traffic traveled thr It might seem intuitive at first because of the symmetry, but our subprocess subsystem is actually pretty asymmetric. We have very fine control over what we send to stdin because it takes explicit calls to an FD; less so for stdout -- it's very easy for a library to "accidentally" log to stdout if misconfigured. I don't think symmetry is a compelling enough reason here. http://gerrit.cloudera.org:8080/#/c/15574/2/src/kudu/ranger/ranger_client.cc File src/kudu/ranger/ranger_client.cc: http://gerrit.cloudera.org:8080/#/c/15574/2/src/kudu/ranger/ranger_client.cc@65 PS2, Line 65: rime > Should probably call this a FIFO rather than a pipe, as you're creating it Done http://gerrit.cloudera.org:8080/#/c/15574/2/src/kudu/ranger/ranger_client.cc@247 PS2, Line 247: HISTINIT(server_outbound_queue_size_bytes, ranger_server_outbound_queue_size_bytes); > warning: parameter 'env' is unused [misc-unused-parameters] Done http://gerrit.cloudera.org:8080/#/c/15574/2/src/kudu/subprocess/server.cc File src/kudu/subprocess/server.cc: http://gerrit.cloudera.org:8080/#/c/15574/2/src/kudu/subprocess/server.cc@99 PS2, Line 99: receiver_file_(receiver_file), > Could you indirect this via a new function in Env (with a small unit test i Done http://gerrit.cloudera.org:8080/#/c/15574/2/src/kudu/subprocess/server.cc@153 PS2, Line 153: &responder_threads_[i])); > Why is canonicalization important? Don't mkfifo() and open() understand how Yeah, it's not. http://gerrit.cloudera.org:8080/#/c/15574/2/src/kudu/subprocess/server.cc@156 PS2, Line 156: [this]() { this->ReceiveMessagesThread(); }, : &read_thread_)); > Will this be an issue for a mini cluster with multiple masters? Done http://gerrit.cloudera.org:8080/#/c/15574/2/src/kudu/subprocess/subprocess_proxy.h File src/kudu/subprocess/subprocess_proxy.h: http://gerrit.cloudera.org:8080/#/c/15574/2/src/kudu/subprocess/subprocess_proxy.h@43 PS2, Line 43: std::vector<std::string> argv, const scoped_refptr<MetricEntity>& entity) > warning: the parameter 'argv' is copied for each invocation but only used a Ack -- To view, visit http://gerrit.cloudera.org:8080/15574 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I63e91a090fe196713a013d28301c7980e452456c Gerrit-Change-Number: 15574 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Sat, 28 Mar 2020 10:31:27 +0000 Gerrit-HasComments: Yes
