----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27516/#review60098 -----------------------------------------------------------
src/slave/containerizer/fetcher.hpp <https://reviews.apache.org/r/27516/#comment101438> Really minor, but I think we should update this comment with s/splice/redirect/ now that we've killed the splicing implementation. src/slave/containerizer/fetcher.hpp <https://reviews.apache.org/r/27516/#comment101439> s/splicing/redirecting/ src/slave/containerizer/fetcher.hpp <https://reviews.apache.org/r/27516/#comment101434> Mind replacing '> >' with '>>' everywhere in your patch please? Thanks! src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/27516/#comment101435> Please put "" includes after <> includes. src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/27516/#comment101437> The function fetcherEnviroment should be getting replaced with the new fetcher::environment that is defined above. It looks like fetcherEnvironment in containerizer.hpp|cpp has changed so we should update fetcher::environment with that implementation and then delete it there. src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/27516/#comment101440> You don't need to redirect yourself, you can just use Subprocess::FD(), which properly dups the file descriptor, when you do 'subprocess(...)' above: -------------------------------------------------- Try<Subprocess> fetcher = subprocess( command, Subprocess::PIPE(), stdout.isSome() ? Subprocess::FD(stdout.get()) : Subprocess::PIPE(), stderr.isSome() ? Subprocess::FD(stderr.get()) : Subprocess::PIPE(), environment); if (fetcher.isError()) { return Failure("Failed to execute mesos-fetcher: " + fetcher.error()); } return fetcher.get().status(); -------------------------------------------------- The advantage to using Subprocess::FD() is that the redirection will keep working even if the parent process exits where is io::redirect() only works as long as the parent is still alive. src/slave/containerizer/fetcher.cpp <https://reviews.apache.org/r/27516/#comment101441> Even after making the change above, we shouldn't close the passed in file descriptors. The comment for this function also says that we dup the file descriptors, which implies that we won't be closing them because we're creating our own copy so we can control our copy's lifetime. In general, the semantics that I've seen compose well are always dup'ing the descriptors ourselves (if we want them to outlive the function call) and then letting the caller close any file descriptors they passed in on their own accord. It's much easier to reason about file descriptor lifetimes with these semantics. Even better would be to have an FD abstraction that does reference counting, but unfortunately we don't have one of those yet. src/slave/containerizer/mesos/containerizer.cpp <https://reviews.apache.org/r/27516/#comment101444> Why does this need to be a Future? It looks like you're chaining this below via a 'then', which means you can just use Option<int> since a 'then' only invokes this if the Future is ready. The fact that you're always doing a 'get()' on the future too implies this precondition. src/slave/containerizer/mesos/containerizer.cpp <https://reviews.apache.org/r/27516/#comment101445> Since we're no longer doing io::redirect here it would be great to update this comment to refer to fetcher::run instead. You'll likely need to reword the comment slightly, but I think the gist of it is that we could update fetcher::run to take paths instead of file descriptors and then use Subprocess::PATH() instead of Subprocess::FD(). Let's not actually do that now, but just update the comment. ;-) src/slave/containerizer/mesos/containerizer.cpp <https://reviews.apache.org/r/27516/#comment101446> Please add a space between :" on this line. Same below please. src/slave/containerizer/mesos/containerizer.cpp <https://reviews.apache.org/r/27516/#comment101442> Given the code above this would be a double close, which are notoriously hard to debug! We should definitely keep the closes here and remove the close up in fetcher::run (see the comment there). src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/27516/#comment101447> We separate these with a newline just like we separate process, stout, mesos, etc. Same below with 'slave/containerizer/fetcher.hpp' and 'slave/flags.hpp'. Basically, we put 'dirname' things together, so: a/b.hpp a/c.hpp a/d/e.hpp a/d/f.hpp a/d/g/h.hpp a/i/j.hpp src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/27516/#comment101448> This is not your bug, but would you mind making the old tests consistent with the tests that were added in this patch by changing the code to: CommandInfo::URI* uri = commandInfo.add_uris(); uri->set_value(...); uri->set_executable(...); src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/27516/#comment101443> This is a good example of where closing the "original" file descriptors in fetcher::run ends up being an awkward semantics, because someone using that function would have just closed stdout and stderr! - Benjamin Hindman On Nov. 3, 2014, 4:36 p.m., Bernd Mathiske wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27516/ > ----------------------------------------------------------- > > (Updated Nov. 3, 2014, 4:36 p.m.) > > > Review request for mesos and Benjamin Hindman. > > > Bugs: MESOS-1316 > https://issues.apache.org/jira/browse/MESOS-1316 > > > Repository: mesos-git > > > Description > ------- > > Manually rebasing and re-editing https://reviews.apache.org/r/21233/, which > is supposed to be replaced now by this patch. > > Original description: "To test the mesos-fetcher (and the setting of the > environment) more cleanly I did some refactoring into a 'fetcher' namespace." > > Also moved fetcher environment tests to fetcher test file. Added two fetcher > tests. > > > Diffs > ----- > > src/Makefile.am e6a07150c10b9fa040143e394b2f913a18eeebc1 > src/launcher/fetcher.cpp 9323c28237010fa065ef34d74435c151ded530a8 > src/slave/containerizer/fetcher.hpp PRE-CREATION > src/slave/containerizer/fetcher.cpp PRE-CREATION > src/slave/containerizer/mesos/containerizer.cpp > d4b08f54d6feb453f3a9d27ca54c867176e62102 > src/tests/containerizer_tests.cpp 2c90d2fc18a3268c55b6dfe98699bfb36d093983 > src/tests/fetcher_tests.cpp d7754009a59fedb43e3422c56b3a786ce80164aa > > Diff: https://reviews.apache.org/r/27516/diff/ > > > Testing > ------- > > make check on Mac OS 10.10 and Ubuntu 14.4. > > In total, 3 tests fail: ExamplesTest.NoExecutorFramework, > ExamplesTest.JavaFramework > , ExamplesTest.PythonFramework. It is strongly suspected that those are > unrelated to this code change and just generally flaky. > > > Thanks, > > Bernd Mathiske > >
