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 2013/11/17 00:13:58 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 Nov. 16, 2013, 11:13 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Niklas Nielsen.
Repository: mesos-git
Description (updated)
-------
Offer an execvp like interface for running tasks.
Review: https://reviews.apache.org/r/15542
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp f6bbf5e00a810affd8cb6f828d1f306dc8bf3051
include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
src/common/protobuf_utils.hpp c83cbd693abf87089288d03f01ce06200a5d9245
src/examples/python/test_framework.py deca48e779ae099424fa73bb9a8ac5c419c5faf1
src/exec/exec.cpp c866838c90901610803df026329060c170d9f21a
src/launcher/executor.cpp b73ab479500a7347a38ba53acecfab9229f1080d
src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
src/master/http.cpp deb5c97d1a9d130ca776fda3bfb173d8682eb364
src/master/master.cpp abab6cede01cabcff10c13caf766a9ecde0891e0
src/messages/messages.proto 71f68a0de2362dd367b04f82853d04fd8c173cff
src/slave/monitor.cpp dd25855fa2f9f71f7ddbd0039c366f50f7eac093
src/slave/process_isolator.cpp a6e9ed6a654972e8a51a9a033052e02ce44fe3e4
src/slave/slave.hpp b39eaf424ada1159f2ff412c0fb3b1738e0e9de5
src/slave/slave.cpp bb98fce41ec6ed837af3384e95cd2b0567317ea9
src/webui/master/static/framework.html 92f01ad2e2fbbabaca3a01d325a83f1d8cb25ffa
src/webui/master/static/frameworks.html 834baf942b3173cef14cc8b1a648f353844f9282
src/webui/master/static/home.html bccffd907954a38353b3e2af319d2659ba0f725f
src/webui/master/static/index.html 929cb0e9eca6825c2fe5b64a392045de6cfd8562
src/webui/master/static/js/app.js 163c50b6cb88f33a23a1a3446d1a88070be26eab
src/webui/master/static/js/controllers.js 1c2e73aea69432c4bb3e2062e28b193973d88937
src/webui/master/static/offers.html 215d65e1ad30fb0df06bcf05cec797d60672ec66
Diff: https://reviews.apache.org/r/15542/diff/
Testing (updated)
-------
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 Nov. 17, 2013, 7:33 p.m., Ben Mahler wrote:
> > Looks like this is in need of a rebase against your local master branch?
I went ahead and rebased. There were no commit conflicts and `make check` ran okay.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review29025
-----------------------------------------------------------
On Nov. 18, 2013, 1:31 a.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Nov. 18, 2013, 1:31 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 a6e9ed6a654972e8a51a9a033052e02ce44fe3e4
>
> 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 Nov. 17, 2013, 7:33 p.m., Ben Mahler wrote:
> > include/mesos/mesos.proto, lines 126-134
> > <https://reviews.apache.org/r/15542/diff/2/?file=386604#file386604line126>
> >
> > Is the goal of your change here to provide a way to explicitly add arguments rather than having to carefully construct the value string?
> >
> > It may be sufficient to add an 'repeated string arguments' below the 'required string value'. We would then behave differently if arguments is non-empty, preserving backwards compatibility and allowing people to explicitly add arguments, what do you think?
The goal of the change is that the caller can pass a command and optional arguments without having to carefully construct either the command string or the argument strings.
Imagine a scenario where a framework wishes to run a command without arguments that is stored in a user's folder on their Mac. The directory is named "Company Stuff" and the command is "do_stats.py". The path is something like "/Users/Bob Marley/Company Stuff/do_stats.py" and following your proposal, if arguments aren't passed, then it has to be escaped.
As far as I can tell, the changes is backwards compatible -- if there is no execv field then everything is handled the old way, with sh -c. It seems reasonable that people can opt in to shell if they know they want it.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review29025
-----------------------------------------------------------
On Nov. 16, 2013, 11:13 p.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Nov. 16, 2013, 11:13 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
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp f6bbf5e00a810affd8cb6f828d1f306dc8bf3051
> include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
> src/common/protobuf_utils.hpp c83cbd693abf87089288d03f01ce06200a5d9245
> src/examples/python/test_framework.py deca48e779ae099424fa73bb9a8ac5c419c5faf1
> src/exec/exec.cpp c866838c90901610803df026329060c170d9f21a
> src/launcher/executor.cpp b73ab479500a7347a38ba53acecfab9229f1080d
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/master/http.cpp deb5c97d1a9d130ca776fda3bfb173d8682eb364
> src/master/master.cpp abab6cede01cabcff10c13caf766a9ecde0891e0
> src/messages/messages.proto 71f68a0de2362dd367b04f82853d04fd8c173cff
> src/slave/monitor.cpp dd25855fa2f9f71f7ddbd0039c366f50f7eac093
> src/slave/process_isolator.cpp a6e9ed6a654972e8a51a9a033052e02ce44fe3e4
> src/slave/slave.hpp b39eaf424ada1159f2ff412c0fb3b1738e0e9de5
> src/slave/slave.cpp bb98fce41ec6ed837af3384e95cd2b0567317ea9
> src/webui/master/static/framework.html 92f01ad2e2fbbabaca3a01d325a83f1d8cb25ffa
> src/webui/master/static/frameworks.html 834baf942b3173cef14cc8b1a648f353844f9282
> src/webui/master/static/home.html bccffd907954a38353b3e2af319d2659ba0f725f
> src/webui/master/static/index.html 929cb0e9eca6825c2fe5b64a392045de6cfd8562
> src/webui/master/static/js/app.js 163c50b6cb88f33a23a1a3446d1a88070be26eab
> src/webui/master/static/js/controllers.js 1c2e73aea69432c4bb3e2062e28b193973d88937
> src/webui/master/static/offers.html 215d65e1ad30fb0df06bcf05cec797d60672ec66
>
> 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/#review29025
-----------------------------------------------------------
Looks like this is in need of a rebase against your local master branch?
include/mesos/mesos.proto
<https://reviews.apache.org/r/15542/#comment56106>
Is the goal of your change here to provide a way to explicitly add arguments rather than having to carefully construct the value string?
It may be sufficient to add an 'repeated string arguments' below the 'required string value'. We would then behave differently if arguments is non-empty, preserving backwards compatibility and allowing people to explicitly add arguments, what do you think?
- Ben Mahler
On Nov. 16, 2013, 11:13 p.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Nov. 16, 2013, 11:13 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
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp f6bbf5e00a810affd8cb6f828d1f306dc8bf3051
> include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34
> src/common/protobuf_utils.hpp c83cbd693abf87089288d03f01ce06200a5d9245
> src/examples/python/test_framework.py deca48e779ae099424fa73bb9a8ac5c419c5faf1
> src/exec/exec.cpp c866838c90901610803df026329060c170d9f21a
> src/launcher/executor.cpp b73ab479500a7347a38ba53acecfab9229f1080d
> src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af
> src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf
> src/master/http.cpp deb5c97d1a9d130ca776fda3bfb173d8682eb364
> src/master/master.cpp abab6cede01cabcff10c13caf766a9ecde0891e0
> src/messages/messages.proto 71f68a0de2362dd367b04f82853d04fd8c173cff
> src/slave/monitor.cpp dd25855fa2f9f71f7ddbd0039c366f50f7eac093
> src/slave/process_isolator.cpp a6e9ed6a654972e8a51a9a033052e02ce44fe3e4
> src/slave/slave.hpp b39eaf424ada1159f2ff412c0fb3b1738e0e9de5
> src/slave/slave.cpp bb98fce41ec6ed837af3384e95cd2b0567317ea9
> src/webui/master/static/framework.html 92f01ad2e2fbbabaca3a01d325a83f1d8cb25ffa
> src/webui/master/static/frameworks.html 834baf942b3173cef14cc8b1a648f353844f9282
> src/webui/master/static/home.html bccffd907954a38353b3e2af319d2659ba0f725f
> src/webui/master/static/index.html 929cb0e9eca6825c2fe5b64a392045de6cfd8562
> src/webui/master/static/js/app.js 163c50b6cb88f33a23a1a3446d1a88070be26eab
> src/webui/master/static/js/controllers.js 1c2e73aea69432c4bb3e2062e28b193973d88937
> src/webui/master/static/offers.html 215d65e1ad30fb0df06bcf05cec797d60672ec66
>
> 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>.
> On Nov. 18, 2013, 6:13 p.m., Ben Mahler wrote:
> > include/mesos/mesos.proto, lines 126-134
> > <https://reviews.apache.org/r/15542/diff/3/?file=387242#file387242line126>
> >
> > Thanks for clarifying!
> >
> > Rather than offer two ways of doing it, let's mark the old one as deprecated so that we can eventually have frameworks only using this improved version. Or is there a reason to keep both versions indefinitely?
> >
> > How about we clean up the naming a little bit? ExecV seems to suggest too much about the implementation, what about s/ExecV/Command/ and s/args/arguments/ (we tend to avoid abbreviations):
> >
> > message Command {
> > required string command = 1;
> > repeated string arguments = 2;
> > }
> >
> > ...
> > required string value = 3; // Deprecated, please use command instead.
> > optional Command command = 4;
>
> Jason Dusek wrote:
> I would be happy to change the names as you suggest.
>
> Deprecating `value` makes a lot of sense, I will add the comment.
>
> In the future -- maybe in a future update for this review? -- we could add an optional `bytes body` to the Command structure, which would allow users to pass a script -- shell or Python or Lua or PL/SQL -- which the slave could place in a temporary location and mark executable. This would be a good option for frameworks and tools that presently rely on shell control flow and glob expansion. (As does the Hadoop framework, for example.) The name of the file on disk where the `body` is placed would be randomly generated; the `command`, if present, could serve as the name of the command as it would show up in `ps`. (Rewriting $0 is something Postfix, Postgres and many other tools do to make it easier to interpret a process listing.) What do you think of that idea?
If we deprecate "value", how would it work for people that do want shell expansion? Is it Command.command = "sh -c foo" ?
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review29054
-----------------------------------------------------------
On Nov. 18, 2013, 1:31 a.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Nov. 18, 2013, 1:31 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 a6e9ed6a654972e8a51a9a033052e02ce44fe3e4
>
> 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 Nov. 18, 2013, 6:13 p.m., Ben Mahler wrote:
> > include/mesos/mesos.proto, lines 126-134
> > <https://reviews.apache.org/r/15542/diff/3/?file=387242#file387242line126>
> >
> > Thanks for clarifying!
> >
> > Rather than offer two ways of doing it, let's mark the old one as deprecated so that we can eventually have frameworks only using this improved version. Or is there a reason to keep both versions indefinitely?
> >
> > How about we clean up the naming a little bit? ExecV seems to suggest too much about the implementation, what about s/ExecV/Command/ and s/args/arguments/ (we tend to avoid abbreviations):
> >
> > message Command {
> > required string command = 1;
> > repeated string arguments = 2;
> > }
> >
> > ...
> > required string value = 3; // Deprecated, please use command instead.
> > optional Command command = 4;
>
> Jason Dusek wrote:
> I would be happy to change the names as you suggest.
>
> Deprecating `value` makes a lot of sense, I will add the comment.
>
> In the future -- maybe in a future update for this review? -- we could add an optional `bytes body` to the Command structure, which would allow users to pass a script -- shell or Python or Lua or PL/SQL -- which the slave could place in a temporary location and mark executable. This would be a good option for frameworks and tools that presently rely on shell control flow and glob expansion. (As does the Hadoop framework, for example.) The name of the file on disk where the `body` is placed would be randomly generated; the `command`, if present, could serve as the name of the command as it would show up in `ps`. (Rewriting $0 is something Postfix, Postgres and many other tools do to make it easier to interpret a process listing.) What do you think of that idea?
>
> Vinod Kone wrote:
> If we deprecate "value", how would it work for people that do want shell expansion? Is it Command.command = "sh -c foo" ?
Here is a way to get shell expansion with the Command message:
Command.command = "/bin/sh"
Command.arguments = ["-c", "shell command with $vars and gl*bs"]
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review29054
-----------------------------------------------------------
On Nov. 18, 2013, 1:31 a.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Nov. 18, 2013, 1:31 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 a6e9ed6a654972e8a51a9a033052e02ce44fe3e4
>
> 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 Nov. 18, 2013, 6:13 p.m., Ben Mahler wrote:
> > include/mesos/mesos.proto, lines 126-134
> > <https://reviews.apache.org/r/15542/diff/3/?file=387242#file387242line126>
> >
> > Thanks for clarifying!
> >
> > Rather than offer two ways of doing it, let's mark the old one as deprecated so that we can eventually have frameworks only using this improved version. Or is there a reason to keep both versions indefinitely?
> >
> > How about we clean up the naming a little bit? ExecV seems to suggest too much about the implementation, what about s/ExecV/Command/ and s/args/arguments/ (we tend to avoid abbreviations):
> >
> > message Command {
> > required string command = 1;
> > repeated string arguments = 2;
> > }
> >
> > ...
> > required string value = 3; // Deprecated, please use command instead.
> > optional Command command = 4;
I would be happy to change the names as you suggest.
Deprecating `value` makes a lot of sense, I will add the comment.
In the future -- maybe in a future update for this review? -- we could add an optional `bytes body` to the Command structure, which would allow users to pass a script -- shell or Python or Lua or PL/SQL -- which the slave could place in a temporary location and mark executable. This would be a good option for frameworks and tools that presently rely on shell control flow and glob expansion. (As does the Hadoop framework, for example.) The name of the file on disk where the `body` is placed would be randomly generated; the `command`, if present, could serve as the name of the command as it would show up in `ps`. (Rewriting $0 is something Postfix, Postgres and many other tools do to make it easier to interpret a process listing.) What do you think of that idea?
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review29054
-----------------------------------------------------------
On Nov. 18, 2013, 1:31 a.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Nov. 18, 2013, 1:31 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 a6e9ed6a654972e8a51a9a033052e02ce44fe3e4
>
> 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/#review29054
-----------------------------------------------------------
include/mesos/mesos.proto
<https://reviews.apache.org/r/15542/#comment56134>
Thanks for clarifying!
Rather than offer two ways of doing it, let's mark the old one as deprecated so that we can eventually have frameworks only using this improved version. Or is there a reason to keep both versions indefinitely?
How about we clean up the naming a little bit? ExecV seems to suggest too much about the implementation, what about s/ExecV/Command/ and s/args/arguments/ (we tend to avoid abbreviations):
message Command {
required string command = 1;
repeated string arguments = 2;
}
...
required string value = 3; // Deprecated, please use command instead.
optional Command command = 4;
- Ben Mahler
On Nov. 18, 2013, 1:31 a.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Nov. 18, 2013, 1:31 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 a6e9ed6a654972e8a51a9a033052e02ce44fe3e4
>
> 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
>
>
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 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 Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review29934
-----------------------------------------------------------
I've been too overloaded to take a look at this, sorry Jason. Ben Mahler and Vinod Kone, any more thoughts?
- Benjamin Hindman
On Nov. 25, 2013, 5:13 a.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Nov. 25, 2013, 5:13 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 a6e9ed6a654972e8a51a9a033052e02ce44fe3e4
>
> 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 Dec. 7, 2013, 7:47 p.m., Ben Mahler wrote:
> > src/launcher/executor.cpp, line 147
> > <https://reviews.apache.org/r/15542/diff/8/?file=390596#file390596line147>
> >
> > s/std::string/string/
> >
> > Do you want a space after the colon to print nicely?
The space is added before each argument (`commandMessage += " " + arg`) before it is printed. This includes the first argument.
> On Dec. 7, 2013, 7:47 p.m., Ben Mahler wrote:
> > src/examples/python/test_framework.py, line 124
> > <https://reviews.apache.org/r/15542/diff/8/?file=390595#file390595line124>
> >
> > Can you kill this line?
I have set command.value to an empty string. Note that it is a required field in the proto.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review29937
-----------------------------------------------------------
On Nov. 25, 2013, 5:13 a.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Nov. 25, 2013, 5:13 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>.
> On Dec. 7, 2013, 7:47 p.m., Ben Mahler wrote:
> >
Looks pretty good! Mostly just some style nits at this point
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review29937
-----------------------------------------------------------
On Nov. 25, 2013, 5:13 a.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Nov. 25, 2013, 5:13 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 a6e9ed6a654972e8a51a9a033052e02ce44fe3e4
>
> 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/#review29937
-----------------------------------------------------------
src/examples/python/test_framework.py
<https://reviews.apache.org/r/15542/#comment57419>
Can you kill this line?
src/launcher/executor.cpp
<https://reviews.apache.org/r/15542/#comment57421>
s/std::vector/vector/ (Add a using clause above)
s/std::string/string/
src/launcher/executor.cpp
<https://reviews.apache.org/r/15542/#comment57423>
I believe you need an std::back_inserter to use std::copy like this, see:
http://stackoverflow.com/a/5034274
Or more simply:
args.insert(
args.begin(),
command.arguments.begin(),
command.arguemnts.end());
src/launcher/executor.cpp
<https://reviews.apache.org/r/15542/#comment57425>
s/std::string/string/
Do you want a space after the colon to print nicely?
src/launcher/executor.cpp
<https://reviews.apache.org/r/15542/#comment57426>
s/std::vector/vector/
src/launcher/executor.cpp
<https://reviews.apache.org/r/15542/#comment57427>
s/string/string&/
src/launcher/executor.cpp
<https://reviews.apache.org/r/15542/#comment57428>
What's this for? c_str is not async signal safe?
The cast is async signal safe, can you move it below?
src/launcher/launcher.cpp
<https://reviews.apache.org/r/15542/#comment57429>
Ditto my comments from above.
src/launcher/launcher.cpp
<https://reviews.apache.org/r/15542/#comment57430>
Remove std:: (in general we use using clauses in cpp files).
src/launcher/launcher.cpp
<https://reviews.apache.org/r/15542/#comment57431>
string&
src/slave/process_isolator.cpp
<https://reviews.apache.org/r/15542/#comment57444>
Remove std:: qualifiers.
src/slave/process_isolator.cpp
<https://reviews.apache.org/r/15542/#comment57446>
Ditto on needing a back inserter or just using insert().
src/slave/process_isolator.cpp
<https://reviews.apache.org/r/15542/#comment57447>
Kill std:: qualifier.
src/slave/process_isolator.cpp
<https://reviews.apache.org/r/15542/#comment57448>
string&
src/slave/process_isolator.cpp
<https://reviews.apache.org/r/15542/#comment57449>
Can we do this cast down below where it's used?
- Ben Mahler
On Nov. 25, 2013, 5:13 a.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Nov. 25, 2013, 5:13 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 a6e9ed6a654972e8a51a9a033052e02ce44fe3e4
>
> 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 Nov. 25, 2013, 5:13 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 a6e9ed6a654972e8a51a9a033052e02ce44fe3e4
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 Nov. 23, 2013, 2:12 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 a6e9ed6a654972e8a51a9a033052e02ce44fe3e4
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 Nov. 22, 2013, 10:16 p.m., Niklas Nielsen wrote:
> > src/launcher/executor.cpp, line 155
> > <https://reviews.apache.org/r/15542/diff/6/?file=389565#file389565line155>
> >
> > A short comment on exec's char const requirement would be nice. This is the right way but looks like a mistake when you skim it :)
Added in latest update to the patch (r8).
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review29319
-----------------------------------------------------------
On Nov. 25, 2013, 5:13 a.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Nov. 25, 2013, 5:13 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 a6e9ed6a654972e8a51a9a033052e02ce44fe3e4
>
> 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 Nov. 22, 2013, 10:16 p.m., Niklas Nielsen wrote:
> > src/launcher/executor.cpp, line 155
> > <https://reviews.apache.org/r/15542/diff/6/?file=389565#file389565line155>
> >
> > A short comment on exec's char const requirement would be nice. This is the right way but looks like a mistake when you skim it :)
>
> Jason Dusek wrote:
> Added in latest update to the patch (r8).
Bump
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review29319
-----------------------------------------------------------
On Nov. 25, 2013, 5:13 a.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Nov. 25, 2013, 5:13 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 a6e9ed6a654972e8a51a9a033052e02ce44fe3e4
>
> 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/#review29319
-----------------------------------------------------------
Ship it!
Would it make sense to follow up with a test (in src/tests) which exercises and validates the new convention?
src/launcher/executor.cpp
<https://reviews.apache.org/r/15542/#comment56486>
Can you wrap to 2 indents? So it becomes
std::copy(
command.arguments().begin(),
command.arguments().end(),
args.end());
src/launcher/executor.cpp
<https://reviews.apache.org/r/15542/#comment56487>
A short comment on exec's char const requirement would be nice. This is the right way but looks like a mistake when you skim it :)
src/launcher/executor.cpp
<https://reviews.apache.org/r/15542/#comment56488>
Can you wrap in the same way as mentioned above?
src/launcher/launcher.cpp
<https://reviews.apache.org/r/15542/#comment56489>
As mentioned above
- Niklas Nielsen
On Nov. 21, 2013, 6:51 p.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2013, 6:51 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 a6e9ed6a654972e8a51a9a033052e02ce44fe3e4
>
> 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 Nov. 21, 2013, 6:51 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Niklas Nielsen.
Changes
-------
I've moved all the std::* stuff that I added to above the fork() in src/launcher/executor.cpp and src/slave/process_isolator.cpp. In addition, I handled some instances of C++ style output that were pre-existent in the code base.
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 a6e9ed6a654972e8a51a9a033052e02ce44fe3e4
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 Nov. 20, 2013, 9:34 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 a6e9ed6a654972e8a51a9a033052e02ce44fe3e4
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 Nov. 18, 2013, 11:57 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 a6e9ed6a654972e8a51a9a033052e02ce44fe3e4
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 Nov. 18, 2013, 8:01 p.m., Vinod Kone wrote:
> > src/launcher/launcher.cpp, lines 188-203
> > <https://reviews.apache.org/r/15542/diff/3/?file=387245#file387245line188>
> >
> > Ditto. Is this async signal safe?
This function is called only in src/launcher/main.cpp, where there is no call to fork().
> On Nov. 18, 2013, 8:01 p.m., Vinod Kone wrote:
> > src/launcher/executor.cpp, lines 186-190
> > <https://reviews.apache.org/r/15542/diff/3/?file=387244#file387244line186>
> >
> > Why not print this in the above loop?
The string construction has been moved to the loop. Output is handled by a write() call in the child.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review29064
-----------------------------------------------------------
On Nov. 21, 2013, 6:51 p.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2013, 6:51 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 a6e9ed6a654972e8a51a9a033052e02ce44fe3e4
>
> 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 Ian Downes <ia...@gmail.com>.
> On Nov. 18, 2013, 8:01 p.m., Vinod Kone wrote:
> > src/slave/process_isolator.cpp, line 236
> > <https://reviews.apache.org/r/15542/diff/3/?file=387247#file387247line236>
> >
> > Is std::copy async signal safe? Doesn't look like it from this list?
> >
> > https://www.securecoding.cert.org/confluence/display/seccode/SIG30-C.+Call+only+asynchronous-safe+functions+within+signal+handlers
> >
> >
Just a heads-up: We're working on refactoring the whole isolator and modularizing the isolation and launching components. It's some time away from being ready so this execvp feature is sure to be committed first but I'll be sure to include it.
We've ensured async signal safety by having almost nothing between the fork() and the exec() and precomputing anything in the parent before the fork().
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review29064
-----------------------------------------------------------
On Nov. 18, 2013, 11:57 p.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Nov. 18, 2013, 11:57 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 a6e9ed6a654972e8a51a9a033052e02ce44fe3e4
>
> 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 Nov. 18, 2013, 8:01 p.m., Vinod Kone wrote:
> > src/launcher/executor.cpp, lines 168-192
> > <https://reviews.apache.org/r/15542/diff/3/?file=387244#file387244line168>
> >
> > I suspect this is not async signal safe? We have had issues in the past where we were trying to do too much C++ (e.g., ostringstream) between fork() and exec() resulting in dead locks.
We might not even be able to use .push_back() in this context, I gather. For future reference, here's a link that describes the restrictions on code following fork() in a multi-threaded context:
http://pubs.opengroup.org/onlinepubs/009695399/functions/fork.html
"...the child process may only execute async-signal-safe operations..."
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15542/#review29064
-----------------------------------------------------------
On Nov. 18, 2013, 11:57 p.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Nov. 18, 2013, 11:57 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 a6e9ed6a654972e8a51a9a033052e02ce44fe3e4
>
> 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/#review29064
-----------------------------------------------------------
include/mesos/mesos.proto
<https://reviews.apache.org/r/15542/#comment56141>
Can you add documentation about what ExecV is used for? Also, while you are it, mind killing the trailing white space here?
src/launcher/executor.cpp
<https://reviews.apache.org/r/15542/#comment56143>
I suspect this is not async signal safe? We have had issues in the past where we were trying to do too much C++ (e.g., ostringstream) between fork() and exec() resulting in dead locks.
src/launcher/executor.cpp
<https://reviews.apache.org/r/15542/#comment56144>
Why not print this in the above loop?
src/launcher/launcher.cpp
<https://reviews.apache.org/r/15542/#comment56145>
Ditto. Is this async signal safe?
src/slave/process_isolator.cpp
<https://reviews.apache.org/r/15542/#comment56146>
Is std::copy async signal safe? Doesn't look like it from this list?
https://www.securecoding.cert.org/confluence/display/seccode/SIG30-C.+Call+only+asynchronous-safe+functions+within+signal+handlers
- Vinod Kone
On Nov. 18, 2013, 1:31 a.m., Jason Dusek wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15542/
> -----------------------------------------------------------
>
> (Updated Nov. 18, 2013, 1:31 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 a6e9ed6a654972e8a51a9a033052e02ce44fe3e4
>
> 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 Nov. 18, 2013, 1:31 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 a6e9ed6a654972e8a51a9a033052e02ce44fe3e4
Diff: https://reviews.apache.org/r/15542/diff/
Testing
-------
Ran Python test executor and `make check`.
Thanks,
Jason Dusek