----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54293/#review157937 -----------------------------------------------------------
src/slave/containerizer/mesos/io/switchboard.hpp (line 126) <https://reviews.apache.org/r/54293/#comment228606> It's somewhat arbitrary, but can we make this the second to last parameter, in order to group the booleans together? If so, can we do it throughout? src/slave/containerizer/mesos/io/switchboard.hpp (line 178) <https://reviews.apache.org/r/54293/#comment228607> s/termial/terminal src/slave/containerizer/mesos/io/switchboard.cpp (line 247) <https://reviews.apache.org/r/54293/#comment228608> Can you add a comment here about the difference between the two hashsets? Reading top to bottom, it's not clear exactly what each one represents just by looking at the name of the variable. src/slave/containerizer/mesos/io/switchboard.cpp (line 258) <https://reviews.apache.org/r/54293/#comment228609> s/the pseudo/a pseudo src/slave/containerizer/mesos/io/switchboard.cpp (line 277) <https://reviews.apache.org/r/54293/#comment228610> Does it make sense to eventually move this function into stout? src/slave/containerizer/mesos/io/switchboard.cpp (line 283) <https://reviews.apache.org/r/54293/#comment228611> Does it make sense to eventually move this function into stout? src/slave/containerizer/mesos/io/switchboard.cpp (lines 388 - 391) <https://reviews.apache.org/r/54293/#comment228612> I guess you don't need to set the other flags anymore because you rely on their default values now? src/slave/containerizer/mesos/io/switchboard.cpp (lines 439 - 441) <https://reviews.apache.org/r/54293/#comment228613> Can you add a comment about why we do this? When I first looked at it, it wasn't obvious why it was necessary, but I see why it's needed later on (i.e. for future failures). src/slave/containerizer/mesos/io/switchboard.cpp (lines 749 - 750) <https://reviews.apache.org/r/54293/#comment228614> Can you add a quick comment about why we don't need to redirect to `stderr` if we have a tty set up. - Kevin Klues On Dec. 4, 2016, 9:21 p.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54293/ > ----------------------------------------------------------- > > (Updated Dec. 4, 2016, 9:21 p.m.) > > > Review request for mesos, Benjamin Hindman and Kevin Klues. > > > Bugs: MESOS-6470 > https://issues.apache.org/jira/browse/MESOS-6470 > > > Repository: mesos > > > Description > ------- > > Supported TTY in I/O switchboard. > > > Diffs > ----- > > include/mesos/slave/containerizer.proto > 33b4c231cc88a048f3de8ab4b2e0e9eb2140132d > src/slave/containerizer/mesos/containerizer.cpp > a7e2665d4c64b7b322d4ae8d5441e8e777236c59 > src/slave/containerizer/mesos/io/switchboard.hpp > 23c66bb2b05310a12fec24a5e5eebddd370ba0f0 > src/slave/containerizer/mesos/io/switchboard.cpp > 778367a268ec350ed438bc9fe9d359d63bdb5503 > src/slave/containerizer/mesos/io/switchboard_main.cpp > 5800217d3562a53218d966c57a14e8b768643c34 > src/slave/containerizer/mesos/launch.cpp > d78ca4df694712e60a15f1795878653eb5e0414e > src/tests/containerizer/io_switchboard_tests.cpp > d82f22b5c9a8d177577db3aeaae9e571e7e2fb80 > > Diff: https://reviews.apache.org/r/54293/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
