----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15542/#review32671 -----------------------------------------------------------
Ship it! Thanks for making the driver updates, mostly nits below. In response to your 'shell magic' comment earlier, I think we should leave 'value' as non-deprecated given it's present in other subprocess libraries as a convenience (see my comment below). I expect this to be just one more diff based on the comments below. include/mesos/mesos.proto <https://reviews.apache.org/r/15542/#comment61590> Please wrap at 70 characters. include/mesos/mesos.proto <https://reviews.apache.org/r/15542/#comment61588> newline include/mesos/mesos.proto <https://reviews.apache.org/r/15542/#comment61604> Let's update this to reflect that there are two ways to do it, rather than marking 'value' as deprecated, given my response below (sorry for not addressing your concern re: 'shell magic' earlier). include/mesos/mesos.proto <https://reviews.apache.org/r/15542/#comment61605> Let's update this to reflect that there are two ways to do it, rather than marking 'value' as deprecated, given my response below (sorry for not addressing your concern re: 'shell magic' earlier). include/mesos/mesos.proto <https://reviews.apache.org/r/15542/#comment61606> Let's add a TODO to set this to optional after a release cycle. include/mesos/mesos.proto <https://reviews.apache.org/r/15542/#comment61587> newline src/launcher/executor.cpp <https://reviews.apache.org/r/15542/#comment61592> newline src/launcher/executor.cpp <https://reviews.apache.org/r/15542/#comment61595> Can you use STDOUT_FILENO from unistd.h here instead? src/sched/sched.cpp <https://reviews.apache.org/r/15542/#comment61599> Let's add a little more context: // Set CommandInfo.value if CommandInfo.command is present. // This ensures that we can safely upgrade into a version where // CommandInfo.value to optional after a deprecation cycle. src/sched/sched.cpp <https://reviews.apache.org/r/15542/#comment61602> Let's not say that 'value' is deprecated, in response to your earlier comment, most utilities I see provide the convenience of using a string as opposed to explicit list of arguments: Python's subprocess provides a shell=True mechanism for specifying a string: http://docs.python.org/2/library/subprocess.html#popen-constructor Facebook's C++ subprocess utilities provide both the vector and string style versions: https://github.com/facebook/folly/blob/master/folly/Subprocess.h#L21 src/slave/process_isolator.cpp <https://reviews.apache.org/r/15542/#comment61603> newline - Ben Mahler On Jan. 21, 2014, 10:36 p.m., Jason Dusek wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15542/ > ----------------------------------------------------------- > > (Updated Jan. 21, 2014, 10:36 p.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, and Niklas Nielsen. > > > Repository: mesos-git > > > Description > ------- > > Offer an execvp like interface for running tasks. > > Review: https://reviews.apache.org/r/15542 > > > Diffs > ----- > > include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34 > src/examples/python/test_framework.py > deca48e779ae099424fa73bb9a8ac5c419c5faf1 > src/launcher/executor.cpp b73ab479500a7347a38ba53acecfab9229f1080d > src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af > src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf > src/sched/sched.cpp f9028e81d81c6229d07737df2136077bf902a372 > src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01 > > Diff: https://reviews.apache.org/r/15542/diff/ > > > Testing > ------- > > Ran Python test executor and `make check`. > > > Thanks, > > Jason Dusek > >
