----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52058/#review152600 -----------------------------------------------------------
Fix it, then Ship it! src/slave/containerizer/fetcher.cpp (line 732) <https://reviews.apache.org/r/52058/#comment221695> We are constructing the path at least 3 times. How about ``` const string stdoutPath = path::join(info.sandbox_directory(), "stdout"); ``` and use the variable below? Same for stderr. src/slave/containerizer/fetcher.cpp (line 740) <https://reviews.apache.org/r/52058/#comment221696> Same deal. Use `stderrPath`. src/slave/containerizer/fetcher.cpp (line 754) <https://reviews.apache.org/r/52058/#comment221692> Below the line we should add a TODO ``` TODO(megha.sharma): Fetcher should not create seperate stdout/stderr files but rather use FDs prepared by the container logger. See MESOS-6271 for more details. ``` src/slave/containerizer/fetcher.cpp (line 764) <https://reviews.apache.org/r/52058/#comment221674> Missing space before `+`. src/slave/containerizer/fetcher.cpp (line 766) <https://reviews.apache.org/r/52058/#comment221687> s/ with error// The ":" is enough to indicate the error message that follows. src/slave/containerizer/fetcher.cpp (line 781) <https://reviews.apache.org/r/52058/#comment221688> s/ with error// The ":" is enough to indicate the error message that follows. src/tests/containerizer/mesos_containerizer_tests.cpp (line 591) <https://reviews.apache.org/r/52058/#comment221675> "This test verified that" or "Tests that". src/tests/containerizer/mesos_containerizer_tests.cpp (line 610) <https://reviews.apache.org/r/52058/#comment221683> "exit 0" may be a better (more clear w.r.t intention) dummy executor command. src/tests/containerizer/mesos_containerizer_tests.cpp (line 626) <https://reviews.apache.org/r/52058/#comment221678> `os::getuid(user)` didn't work? src/tests/containerizer/mesos_containerizer_tests.cpp (line 631) <https://reviews.apache.org/r/52058/#comment221679> s/std::string/string/ s/stdOutPath/stdoutPath/ stdout is common enough that we should just treat it as one word. src/tests/containerizer/mesos_containerizer_tests.cpp (line 636) <https://reviews.apache.org/r/52058/#comment221680> s/std::string/string/ s/stdErrPath/stderrPath/ src/tests/containerizer/mesos_containerizer_tests.cpp (line 642) <https://reviews.apache.org/r/52058/#comment221676> 2 space indentation. - Jiang Yan Xu On Oct. 13, 2016, 7:33 a.m., Megha Sharma wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52058/ > ----------------------------------------------------------- > > (Updated Oct. 13, 2016, 7:33 a.m.) > > > Review request for mesos and Jiang Yan Xu. > > > Bugs: MESOS-5218 > https://issues.apache.org/jira/browse/MESOS-5218 > > > Repository: mesos > > > Description > ------- > > Fetcher code changes the ownership of entire sandbox directory > recursively to the taskuser and as a resut also changes the > ownership of files laid out by other entities in the sandbox. > > > Diffs > ----- > > src/slave/containerizer/fetcher.cpp > 11104d66e6dd05d8eb1d37a2e3250aca19278110 > src/tests/containerizer/mesos_containerizer_tests.cpp > ad2070246b7fdb90aa6ed02326b5a7eb95365997 > > Diff: https://reviews.apache.org/r/52058/diff/ > > > Testing > ------- > > make check on linux and macos > > > Thanks, > > Megha Sharma > >
