You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jason Dusek <ja...@gmail.com> on 2014/01/01 00:54:19 UTC
Re: Review Request 15542: Offer an execvp like interface for running tasks.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/
-----------------------------------------------------------
(Updated Dec. 31, 2013, 11:54 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 (updated)
-----
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/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
Diff: https://reviews.apache.org/r/15542/diff/
Testing
-------
Ran Python test executor and `make check`.
Thanks,
Jason Dusek
Re: Review Request 15542: Offer an execvp like interface for running tasks.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On March 10, 2014, 1:45 p.m., Niklas Nielsen wrote:
> > Hey Jason,
> >
> > Are the outstanding issues still pending or do you have an up-coming patch? :)
This patch is outdated and target the old isolators (prior to the containerizer API's). Jason, can we close this for now and circle back on it with a new RR?
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review36700
-----------------------------------------------------------
On Jan. 21, 2014, 2: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, 2: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
>
>
Re: Review Request 15542: Offer an execvp like interface for running tasks.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review36700
-----------------------------------------------------------
Hey Jason,
Are the outstanding issues still pending or do you have an up-coming patch? :)
- Niklas Nielsen
On Jan. 21, 2014, 2: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, 2: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
>
>
Re: Review Request 15542: Offer an execvp like interface for running tasks.
Posted by Ben Mahler <be...@gmail.com>.
> On Jan. 23, 2014, 10:45 p.m., Ben Mahler wrote:
> > src/sched/sched.cpp, line 876
> > <https://reviews.apache.org/r/15542/diff/14/?file=433711#file433711line876>
> >
> > 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
> >
> >
Poor phrasing here, what I meant to say was that I see many libraries providing both versions.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review32671
-----------------------------------------------------------
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
>
>
Re: Review Request 15542: Offer an execvp like interface for running tasks.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 15542: Offer an execvp like interface for running tasks.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review33190
-----------------------------------------------------------
Ship it!
include/mesos/mesos.proto
<https://reviews.apache.org/r/15542/#comment62530>
s/extension/extensions/ ?
include/mesos/mesos.proto
<https://reviews.apache.org/r/15542/#comment62532>
+1
src/examples/python/test_framework.py
<https://reviews.apache.org/r/15542/#comment62533>
isn't this the default?
src/launcher/executor.cpp
<https://reviews.apache.org/r/15542/#comment62534>
How come you didn't something simpler like you did in launcher.cpp below?
src/sched/sched.cpp
<https://reviews.apache.org/r/15542/#comment62535>
Pull this up the above comment ?
src/slave/process_isolator.cpp
<https://reviews.apache.org/r/15542/#comment62536>
thank you.
- Vinod Kone
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
>
>
Re: Review Request 15542: Offer an execvp like interface for running tasks.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review36708
-----------------------------------------------------------
Bad patch!
Reviews applied: [15542]
Failed command: git apply --index 15542.patch
Error:
error: patch failed: include/mesos/mesos.proto:109
error: include/mesos/mesos.proto: patch does not apply
error: src/launcher/launcher.cpp: does not exist in index
error: src/launcher/main.cpp: does not exist in index
error: src/slave/process_isolator.cpp: does not exist in index
- Mesos ReviewBot
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
>
>
Re: Review Request 15542: Offer an execvp like interface for running tasks.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review33350
-----------------------------------------------------------
include/mesos/mesos.proto
<https://reviews.apache.org/r/15542/#comment62801>
I missed that we'll need to update the == operator in protobuf_utils.hpp.
- 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
>
>
Re: Review Request 15542: Offer an execvp like interface for running tasks.
Posted by Jason Dusek <ja...@gmail.com>.
-----------------------------------------------------------
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 (updated)
-----
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
Re: Review Request 15542: Offer an execvp like interface for running tasks.
Posted by Jason Dusek <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/
-----------------------------------------------------------
(Updated Jan. 21, 2014, 6:50 a.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 (updated)
-----
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/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
Diff: https://reviews.apache.org/r/15542/diff/
Testing
-------
Ran Python test executor and `make check`.
Thanks,
Jason Dusek
Re: Review Request 15542: Offer an execvp like interface for running tasks.
Posted by Jason Dusek <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/
-----------------------------------------------------------
(Updated Jan. 21, 2014, 6:21 a.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 (updated)
-----
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/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
Diff: https://reviews.apache.org/r/15542/diff/
Testing
-------
Ran Python test executor and `make check`.
Thanks,
Jason Dusek
Re: Review Request 15542: Offer an execvp like interface for running tasks.
Posted by Jason Dusek <ja...@gmail.com>.
> On Jan. 18, 2014, 1:56 a.m., Ben Mahler wrote:
> > src/slave/process_isolator.cpp, line 271
> > <https://reviews.apache.org/r/15542/diff/9/?file=412611#file412611line271>
> >
> > newline
This comment links to Diff 9 but we were on Diff 11 at the time. In this instance, the newline was already present in the new diff.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review32246
-----------------------------------------------------------
On Jan. 21, 2014, 6:21 a.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2014, 6:21 a.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/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
>
> Diff: https://reviews.apache.org/r/15542/diff/
>
>
> Testing
> -------
>
> Ran Python test executor and `make check`.
>
>
> Thanks,
>
> Jason Dusek
>
>
Re: Review Request 15542: Offer an execvp like interface for running tasks.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review32246
-----------------------------------------------------------
Just some style nits below.
src/launcher/executor.cpp
<https://reviews.apache.org/r/15542/#comment60978>
Can you mention that this is an async signal safe way to print to stdout?
src/launcher/executor.cpp
<https://reviews.apache.org/r/15542/#comment60976>
newline here
src/launcher/executor.cpp
<https://reviews.apache.org/r/15542/#comment60977>
newline here
src/launcher/main.cpp
<https://reviews.apache.org/r/15542/#comment60980>
newline
src/slave/process_isolator.cpp
<https://reviews.apache.org/r/15542/#comment60979>
newline
- Ben Mahler
On Jan. 17, 2014, 5:54 a.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2014, 5:54 a.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/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
>
> Diff: https://reviews.apache.org/r/15542/diff/
>
>
> Testing
> -------
>
> Ran Python test executor and `make check`.
>
>
> Thanks,
>
> Jason Dusek
>
>
Re: Review Request 15542: Offer an execvp like interface for running tasks.
Posted by Jason Dusek <ja...@gmail.com>.
> On Jan. 18, 2014, 1:32 a.m., Ben Mahler wrote:
> > include/mesos/mesos.proto, lines 134-137
> > <https://reviews.apache.org/r/15542/diff/11/?file=426622#file426622line134>
> >
> > s/bytes/string on these two
> >
> > The resulting 'executor.command.command.command' is a bit unfortunate, do you foresee other fields in Command?
> >
> > If not, let's just directly embed it in CommandInfo:
> >
> > message CommandInfo {
> > // Setting value is equivalent to:
> > // command = 'sh'
> > // arguments = ['-c', value]
> > //
> > // Deprecated, please use 'command' and 'arguments' instead.
> > required string value = 3;
> >
> > optional string command = 4;
> > repeated string arguments = 5;
> > }
> >
> > It seems that this makes the API a bit cleaner to use, how does this sound?
I will make the switch from 'bytes' to 'string' as you have suggested. I have some doubts about it -- my intuition suggests this will create vexing limitations sometimes -- but many of my fears are allayed by the fact that Protobuf specifies strings as UTF-8.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review32234
-----------------------------------------------------------
On Jan. 21, 2014, 6:50 a.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2014, 6:50 a.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/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
>
> Diff: https://reviews.apache.org/r/15542/diff/
>
>
> Testing
> -------
>
> Ran Python test executor and `make check`.
>
>
> Thanks,
>
> Jason Dusek
>
>
Re: Review Request 15542: Offer an execvp like interface for running tasks.
Posted by Jason Dusek <ja...@gmail.com>.
> On Jan. 18, 2014, 1:32 a.m., Ben Mahler wrote:
> > include/mesos/mesos.proto, line 141
> > <https://reviews.apache.org/r/15542/diff/11/?file=426622#file426622line141>
> >
> > We should try to move this to an optional field to make the semantics more clear to frameworks, but we'll need to go through a few release cycles to do this.
> >
> > We would have compatibility issues if we were to move this to optional immediately, since messages originating from an upgraded framework will not be parseable my the Master / Slave.
> >
> > 1. As a first step, let's modify sched.cpp to set 'value' to an empty string when 'command' is set. This will ensure that when upgrading to a subsequent release, all messages coming from the scheduler driver will have the field set.
> >
> > 2. Subsequently, and let's leave TODOs for this, we'll be able to move this safely to an optional field.
> >
> > Removing it entirely would be a bit trickier, since it's a breaking API change, I would recommend linking a ticket into MESOS-810 to capture this dependency. Sound good?
This seems like a good approach. Please expect results in a day or two.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review32234
-----------------------------------------------------------
On Jan. 21, 2014, 6:50 a.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2014, 6:50 a.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/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
>
> Diff: https://reviews.apache.org/r/15542/diff/
>
>
> Testing
> -------
>
> Ran Python test executor and `make check`.
>
>
> Thanks,
>
> Jason Dusek
>
>
Re: Review Request 15542: Offer an execvp like interface for running tasks.
Posted by Jason Dusek <ja...@gmail.com>.
> On Jan. 18, 2014, 1:32 a.m., Ben Mahler wrote:
> > include/mesos/mesos.proto, lines 134-137
> > <https://reviews.apache.org/r/15542/diff/11/?file=426622#file426622line134>
> >
> > s/bytes/string on these two
> >
> > The resulting 'executor.command.command.command' is a bit unfortunate, do you foresee other fields in Command?
> >
> > If not, let's just directly embed it in CommandInfo:
> >
> > message CommandInfo {
> > // Setting value is equivalent to:
> > // command = 'sh'
> > // arguments = ['-c', value]
> > //
> > // Deprecated, please use 'command' and 'arguments' instead.
> > required string value = 3;
> >
> > optional string command = 4;
> > repeated string arguments = 5;
> > }
> >
> > It seems that this makes the API a bit cleaner to use, how does this sound?
>
> Jason Dusek wrote:
> I will make the switch from 'bytes' to 'string' as you have suggested. I have some doubts about it -- my intuition suggests this will create vexing limitations sometimes -- but many of my fears are allayed by the fact that Protobuf specifies strings as UTF-8.
I foresee but one more field in Command -- an 'optional bytes code' that would contain an executable (a script, an ELF binary, anything executable) to allow larger (64K?) chunks of generated code to be passed, in the manner of the 'userdata' feature that has proven so helpful in Ubuntu/Amazon's bootstrap systems. Being able to pass along a temporary, generated Python or Bash program -- without having to upload it somewhere and then delete it -- would be a real boon.
I don't foresee any problems with adding this field directly to CommandInfo, though.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review32234
-----------------------------------------------------------
On Jan. 21, 2014, 6:50 a.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2014, 6:50 a.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/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
>
> Diff: https://reviews.apache.org/r/15542/diff/
>
>
> Testing
> -------
>
> Ran Python test executor and `make check`.
>
>
> Thanks,
>
> Jason Dusek
>
>
Re: Review Request 15542: Offer an execvp like interface for running tasks.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review32234
-----------------------------------------------------------
Thanks for taking on this API change Jason, they can be a bit of a pain to flush out fully but we're pretty close here!
A few things of note:
1. What is the required upgrade order here? From my observation:
-> Slaves must be upgraded first (in order to correctly receive the new 'argv' style tasks).
-> Masters and Schedulers have no upgrade order required.
We'll need to keep track of this somewhere (0.17.0 JIRA or docs/Upgrades.md) so that users know how to upgrade safely (this will land in 0.17.0).
2. I'd like us to make the initial step to moving 'value' to optional, by modifying the scheduler driver to set 'value' to an empty string when 'command' is set. This would eliminate the need for frameworks to set 'value' to an empty string in a subsequent release.
include/mesos/mesos.proto
<https://reviews.apache.org/r/15542/#comment60973>
Can you s/uri/URI/ in this comment?
include/mesos/mesos.proto
<https://reviews.apache.org/r/15542/#comment60974>
We handle other file extensions as well, we should update this comment to reflect all of the extensions we handle.
include/mesos/mesos.proto
<https://reviews.apache.org/r/15542/#comment60967>
s/bytes/string on these two
The resulting 'executor.command.command.command' is a bit unfortunate, do you foresee other fields in Command?
If not, let's just directly embed it in CommandInfo:
message CommandInfo {
// Setting value is equivalent to:
// command = 'sh'
// arguments = ['-c', value]
//
// Deprecated, please use 'command' and 'arguments' instead.
required string value = 3;
optional string command = 4;
repeated string arguments = 5;
}
It seems that this makes the API a bit cleaner to use, how does this sound?
include/mesos/mesos.proto
<https://reviews.apache.org/r/15542/#comment60961>
We should try to move this to an optional field to make the semantics more clear to frameworks, but we'll need to go through a few release cycles to do this.
We would have compatibility issues if we were to move this to optional immediately, since messages originating from an upgraded framework will not be parseable my the Master / Slave.
1. As a first step, let's modify sched.cpp to set 'value' to an empty string when 'command' is set. This will ensure that when upgrading to a subsequent release, all messages coming from the scheduler driver will have the field set.
2. Subsequently, and let's leave TODOs for this, we'll be able to move this safely to an optional field.
Removing it entirely would be a bit trickier, since it's a breaking API change, I would recommend linking a ticket into MESOS-810 to capture this dependency. Sound good?
- Ben Mahler
On Jan. 17, 2014, 5:54 a.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2014, 5:54 a.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/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
>
> Diff: https://reviews.apache.org/r/15542/diff/
>
>
> Testing
> -------
>
> Ran Python test executor and `make check`.
>
>
> Thanks,
>
> Jason Dusek
>
>
Re: Review Request 15542: Offer an execvp like interface for running tasks.
Posted by Jason Dusek <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/
-----------------------------------------------------------
(Updated Jan. 17, 2014, 5:54 a.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 (updated)
-----
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/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
Diff: https://reviews.apache.org/r/15542/diff/
Testing
-------
Ran Python test executor and `make check`.
Thanks,
Jason Dusek
Re: Review Request 15542: Offer an execvp like interface for running tasks.
Posted by Jason Dusek <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/
-----------------------------------------------------------
(Updated Jan. 15, 2014, 11:06 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 (updated)
-----
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/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
Diff: https://reviews.apache.org/r/15542/diff/
Testing
-------
Ran Python test executor and `make check`.
Thanks,
Jason Dusek
Re: Review Request 15542: Offer an execvp like interface for running tasks.
Posted by Jason Dusek <ja...@gmail.com>.
> On Jan. 10, 2014, 8:43 p.m., Ben Mahler wrote:
> > include/mesos/mesos.proto, line 141
> > <https://reviews.apache.org/r/15542/diff/9/?file=412606#file412606line141>
> >
> > I was wrong about marking this as deprecated, it still seems useful for frameworks that want the shell to search for binaries on the PATH.
> >
> > Can you kill the comment here?
>
> Jason Dusek wrote:
> But execvp searches for binaries on the path, too.
Although searching for binaries on the path might not be a case where the shell is valuable, the similar use case of using globbing to deal with distro tarballs is. However, in this case, people can use `["sh", "-c", "cd hadoop-*/ && bin/hadoop"]`. What is being deprecated is the implicit shell semantics -- let them be explicit instead. It seems wrong to bake shell semantics in to Mesos -- surely implicit shell magic is not the way of the future?
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review31536
-----------------------------------------------------------
On Jan. 17, 2014, 5:54 a.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Jan. 17, 2014, 5:54 a.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/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
>
> Diff: https://reviews.apache.org/r/15542/diff/
>
>
> Testing
> -------
>
> Ran Python test executor and `make check`.
>
>
> Thanks,
>
> Jason Dusek
>
>
Re: Review Request 15542: Offer an execvp like interface for running tasks.
Posted by Jason Dusek <ja...@gmail.com>.
> On Jan. 10, 2014, 8:43 p.m., Ben Mahler wrote:
> > src/launcher/executor.cpp, line 194
> > <https://reviews.apache.org/r/15542/diff/9/?file=412608#file412608line194>
> >
> > Does this compile? Should this be commandMessage?
Sorry. I had post-reviewed from the wrong branch :(
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review31536
-----------------------------------------------------------
On Dec. 31, 2013, 11:54 p.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Dec. 31, 2013, 11:54 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/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
>
> Diff: https://reviews.apache.org/r/15542/diff/
>
>
> Testing
> -------
>
> Ran Python test executor and `make check`.
>
>
> Thanks,
>
> Jason Dusek
>
>
Re: Review Request 15542: Offer an execvp like interface for running tasks.
Posted by Jason Dusek <ja...@gmail.com>.
> On Jan. 10, 2014, 8:43 p.m., Ben Mahler wrote:
> > include/mesos/mesos.proto, line 141
> > <https://reviews.apache.org/r/15542/diff/9/?file=412606#file412606line141>
> >
> > I was wrong about marking this as deprecated, it still seems useful for frameworks that want the shell to search for binaries on the PATH.
> >
> > Can you kill the comment here?
But execvp searches for binaries on the path, too.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review31536
-----------------------------------------------------------
On Dec. 31, 2013, 11:54 p.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Dec. 31, 2013, 11:54 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/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
>
> Diff: https://reviews.apache.org/r/15542/diff/
>
>
> Testing
> -------
>
> Ran Python test executor and `make check`.
>
>
> Thanks,
>
> Jason Dusek
>
>
Re: Review Request 15542: Offer an execvp like interface for running tasks.
Posted by Jason Dusek <ja...@gmail.com>.
> On Jan. 10, 2014, 8:43 p.m., Ben Mahler wrote:
> > src/launcher/executor.cpp, line 148
> > <https://reviews.apache.org/r/15542/diff/9/?file=412608#file412608line148>
> >
> > Where is commandMessage used?
> >
> > Consider:
> > string commandMessage = "command: " + strings::join(" ", args) + "\n";
The variable `commandMessage` is only used to set `commandPrintable`.
> On Jan. 10, 2014, 8:43 p.m., Ben Mahler wrote:
> > src/launcher/executor.cpp, line 196
> > <https://reviews.apache.org/r/15542/diff/9/?file=412608#file412608line196>
> >
> > Since commandVector does not store a vector, what about calling this _argv (since it's really just storing argv in another form for execvp to stomach).
> >
> > Why does this call to execvp need an intermediate variable, whereas the one in launcher does not?
It needs an intermediate value because it seems unwise to assume a dereference like this is async signal safe. Launcher doesn't fork (as far as I can tell) so it's not an issue.
> On Jan. 10, 2014, 8:43 p.m., Ben Mahler wrote:
> > src/slave/process_isolator.cpp, line 272
> > <https://reviews.apache.org/r/15542/diff/9/?file=412611#file412611line272>
> >
> > Ditto, why does this one need the intermediate commandVector, but the one in launcher does not?
It needs an intermediate value because it seems unwise to assume a dereference like this is async signal safe. Launcher doesn't fork (as far as I can tell) so it's not an issue.
> On Jan. 10, 2014, 8:43 p.m., Ben Mahler wrote:
> > src/launcher/launcher.cpp, line 212
> > <https://reviews.apache.org/r/15542/diff/9/?file=412609#file412609line212>
> >
> > Does this need a newline?
> >
> > Consider:
> >
> > string message = "Could not execute: " + strings::join(" ", args) + "\n";
Other calls to `fatalerror` don't append newlines; `fatalerror` seems to add them (`fprintf(stderr, " (%s:%u)\n", ...)`).
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review31536
-----------------------------------------------------------
On Jan. 15, 2014, 11:06 p.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Jan. 15, 2014, 11:06 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/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
>
> Diff: https://reviews.apache.org/r/15542/diff/
>
>
> Testing
> -------
>
> Ran Python test executor and `make check`.
>
>
> Thanks,
>
> Jason Dusek
>
>
Re: Review Request 15542: Offer an execvp like interface for running tasks.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review31536
-----------------------------------------------------------
include/mesos/mesos.proto
<https://reviews.apache.org/r/15542/#comment60065>
I was wrong about marking this as deprecated, it still seems useful for frameworks that want the shell to search for binaries on the PATH.
Can you kill the comment here?
src/launcher/executor.cpp
<https://reviews.apache.org/r/15542/#comment60054>
Where is commandMessage used?
Consider:
string commandMessage = "command: " + strings::join(" ", args) + "\n";
src/launcher/executor.cpp
<https://reviews.apache.org/r/15542/#comment60055>
Does this compile? Should this be commandMessage?
src/launcher/executor.cpp
<https://reviews.apache.org/r/15542/#comment60057>
Since commandVector does not store a vector, what about calling this _argv (since it's really just storing argv in another form for execvp to stomach).
Why does this call to execvp need an intermediate variable, whereas the one in launcher does not?
src/launcher/launcher.cpp
<https://reviews.apache.org/r/15542/#comment60059>
Does this need a newline?
Consider:
string message = "Could not execute: " + strings::join(" ", args) + "\n";
src/slave/process_isolator.cpp
<https://reviews.apache.org/r/15542/#comment60063>
Ditto, why does this one need the intermediate commandVector, but the one in launcher does not?
- Ben Mahler
On Dec. 31, 2013, 11:54 p.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Dec. 31, 2013, 11:54 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/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01
>
> Diff: https://reviews.apache.org/r/15542/diff/
>
>
> Testing
> -------
>
> Ran Python test executor and `make check`.
>
>
> Thanks,
>
> Jason Dusek
>
>