----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54148/#review157474 -----------------------------------------------------------
Fix it, then Ship it! src/slave/containerizer/mesos/io/switchboard.cpp (line 157) <https://reviews.apache.org/r/54148/#comment228072> is not supported src/slave/containerizer/mesos/io/switchboard.cpp (line 196) <https://reviews.apache.org/r/54148/#comment228079> I would remove this, and s/endif/else/ above src/slave/containerizer/mesos/io/switchboard.cpp (line 291) <https://reviews.apache.org/r/54148/#comment228077> Let's use /dev/null here. src/slave/containerizer/mesos/io/switchboard.cpp (line 312) <https://reviews.apache.org/r/54148/#comment228080> NO need for this check. src/slave/containerizer/mesos/io/switchboard.cpp (lines 314 - 315) <https://reviews.apache.org/r/54148/#comment228081> I would add a constructor to Info and use the following here. ``` infos.put(containerId, Owned<Info>(new Info(pid)); ``` src/slave/containerizer/mesos/io/switchboard.cpp (line 346) <https://reviews.apache.org/r/54148/#comment228082> remove the extra space. src/slave/containerizer/mesos/io/switchboard.cpp (lines 349 - 350) <https://reviews.apache.org/r/54148/#comment228089> This should be a TODO here. src/slave/containerizer/mesos/io/switchboard.cpp (line 355) <https://reviews.apache.org/r/54148/#comment228092> I suggest we start the `reap` in `prepare` the moment we create Info, and registered a callback `reaped`. This way, it's easier for us to impl. 'watch' in the future. ``` struct Info { pid_t pid; Future<Option<int>> status; } ``` src/slave/containerizer/mesos/io/switchboard.cpp (line 357) <https://reviews.apache.org/r/54148/#comment228094> You need to check if infos.contains(contianerId) here. See my comments above, in cleanup, the logic should be: ``` if (local || !infos.contains(containerId)) { return Nothing(); } Future<Nothing> status = infos[containerId]->status; infos.erase(containerId); return status; ``` src/slave/containerizer/mesos/io/switchboard.cpp (line 359) <https://reviews.apache.org/r/54148/#comment228088> Ditto. No need for return value. src/slave/containerizer/mesos/io/switchboard_main.cpp (line 56) <https://reviews.apache.org/r/54148/#comment228096> let be defensive here: ``` if (!run.isReady()) { ... } ``` - Jie Yu On Nov. 30, 2016, 6:54 p.m., Kevin Klues wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54148/ > ----------------------------------------------------------- > > (Updated Nov. 30, 2016, 6:54 p.m.) > > > Review request for mesos and Jie Yu. > > > Bugs: MESOS-6639 > https://issues.apache.org/jira/browse/MESOS-6639 > > > Repository: mesos > > > Description > ------- > > We don't currently handle recovering access to the io switchboard > server process after agent restarts. We will add that in a subsequent > commit. > > > Diffs > ----- > > src/Makefile.am d1cc1016b1642c504f52c27623bc12c7ddf07599 > src/slave/containerizer/mesos/io/switchboard.hpp > aaa3a35245b291f6003f519dbf8c0e1b82bc15fd > src/slave/containerizer/mesos/io/switchboard.cpp > 25cbf2447d197134f0753b062b6f4130821005b2 > src/slave/containerizer/mesos/io/switchboard_main.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/54148/diff/ > > > Testing > ------- > > GTEST_FILTER="" make -j check > sudo src/mesos-tests > > [ FAILED ] > bool/UserContainerLoggerTest.ROOT_LOGROTATE_RotateWithSwitchUserTrueOrFalse/0, > where GetParam() = true > > > Thanks, > > Kevin Klues > >
