You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Armand Grillet <ag...@mesosphere.io> on 2017/07/25 15:48:56 UTC
Review Request 61112: Added flag executor_extra_uris to balloon
framework.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61112/
-----------------------------------------------------------
Review request for mesos, Alexander Rukletsov and Alexander Rojas.
Bugs: MESOS-7814
https://issues.apache.org/jira/browse/MESOS-7814
Repository: mesos
Description
-------
This allows to set URIs that will be fetched and executed before
running the executor.
Diffs
-----
src/examples/balloon_framework.cpp b8c8e387bd8592bc1d27ff62fa9fd5397ad71fb9
Diff: https://reviews.apache.org/r/61112/diff/1/
Testing
-------
$ make check
Thanks,
Armand Grillet
Re: Review Request 61112: Added flag executor_extra_uris to balloon
framework.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61112/#review181355
-----------------------------------------------------------
Patch looks great!
Reviews applied: [61110, 61111, 61112]
Passed command: support\windows-build.bat
- Mesos Reviewbot Windows
On July 25, 2017, 3:48 p.m., Armand Grillet wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61112/
> -----------------------------------------------------------
>
> (Updated July 25, 2017, 3:48 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Alexander Rojas.
>
>
> Bugs: MESOS-7814
> https://issues.apache.org/jira/browse/MESOS-7814
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This allows to set URIs that will be fetched and executed before
> running the executor.
>
>
> Diffs
> -----
>
> src/examples/balloon_framework.cpp b8c8e387bd8592bc1d27ff62fa9fd5397ad71fb9
>
>
> Diff: https://reviews.apache.org/r/61112/diff/1/
>
>
> Testing
> -------
>
> $ make check
>
>
> Thanks,
>
> Armand Grillet
>
>
Re: Review Request 61112: Added flag executor_extra_uris to balloon
framework.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On July 25, 2017, 7:08 p.m., Till Toenshoff wrote:
> > src/examples/balloon_framework.cpp
> > Lines 120-124 (patched)
> > <https://reviews.apache.org/r/61112/diff/1/?file=1782174#file1782174line120>
> >
> > I would suggest to use a different approach here -- instead of allowing multiple URIs to be used for the executor artefact, lets simply go for using archives in those cases.
> >
> > Now the issue preventing us to do so here is https://github.com/apache/mesos/blob/cd3380c4e9521b4b26f9030658816eee7a4b89a1/src/examples/balloon_framework.cpp#L493
> >
> > Hence my suggestion is to add a new flag called e.g. `executor_executable` which can be used if we want the fetcher to make non archive downloads `chmod` into `+X`. Whenever that flag is not set, we can now allow archive downloads and achieve what we want without having to;
> > - trigger multiple downloads
> > - accept depending libraries to be `chmod` into `+X` which wont trigger failures but still is nasty
Another approach would be to delegate URI specification to the user. A similar approach can be seen in e.g. [1]. This way we will allow for both scenarios: multiple URIs and a single archive URI. The only disadvantage I see here is overcomplicating API for a fairly straightforward framework. Having said that, it seems to me a fair price, given we've already seen requests for specifying a command and an extra library.
[1] https://github.com/apache/mesos/blob/95fe8b367a94da6da0a580026519bf07a4f65ec7/src/cli/execute.cpp#L104
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61112/#review181366
-----------------------------------------------------------
On July 25, 2017, 3:48 p.m., Armand Grillet wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61112/
> -----------------------------------------------------------
>
> (Updated July 25, 2017, 3:48 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Alexander Rojas.
>
>
> Bugs: MESOS-7814
> https://issues.apache.org/jira/browse/MESOS-7814
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This allows to set URIs that will be fetched and executed before
> running the executor.
>
>
> Diffs
> -----
>
> src/examples/balloon_framework.cpp b8c8e387bd8592bc1d27ff62fa9fd5397ad71fb9
>
>
> Diff: https://reviews.apache.org/r/61112/diff/1/
>
>
> Testing
> -------
>
> $ make check
>
>
> Thanks,
>
> Armand Grillet
>
>
Re: Review Request 61112: Added flag executor_extra_uris to balloon
framework.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61112/#review181366
-----------------------------------------------------------
src/examples/balloon_framework.cpp
Lines 120-124 (patched)
<https://reviews.apache.org/r/61112/#comment256905>
I would suggest to use a different approach here -- instead of allowing multiple URIs to be used for the executor artefact, lets simply go for using archives in those cases.
Now the issue preventing us to do so here is https://github.com/apache/mesos/blob/cd3380c4e9521b4b26f9030658816eee7a4b89a1/src/examples/balloon_framework.cpp#L493
Hence my suggestion is to add a new flag called e.g. `executor_executable` which can be used if we want the fetcher to make non archive downloads `chmod` into `+X`. Whenever that flag is not set, we can now allow archive downloads and achieve what we want without having to;
- trigger multiple downloads
- accept depending libraries to be `chmod` into `+X` which wont trigger failures but still is nasty
- Till Toenshoff
On July 25, 2017, 3:48 p.m., Armand Grillet wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61112/
> -----------------------------------------------------------
>
> (Updated July 25, 2017, 3:48 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Alexander Rojas.
>
>
> Bugs: MESOS-7814
> https://issues.apache.org/jira/browse/MESOS-7814
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This allows to set URIs that will be fetched and executed before
> running the executor.
>
>
> Diffs
> -----
>
> src/examples/balloon_framework.cpp b8c8e387bd8592bc1d27ff62fa9fd5397ad71fb9
>
>
> Diff: https://reviews.apache.org/r/61112/diff/1/
>
>
> Testing
> -------
>
> $ make check
>
>
> Thanks,
>
> Armand Grillet
>
>
Re: Review Request 61112: Added executor_uris flag to long lived and
balloon frameworks.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61112/#review181892
-----------------------------------------------------------
Fix it, then Ship it!
I will fix the outstanding issues and commit this for you.
src/examples/balloon_framework.cpp
Lines 46 (patched)
<https://reviews.apache.org/r/61112/#comment257681>
Looks like we should `#include <stout/protobuf.hpp>` instead of this file.
src/examples/balloon_framework.cpp
Line 109 (original), 114 (patched)
<https://reviews.apache.org/r/61112/#comment257682>
Let's mention this flag is deprecated.
src/examples/balloon_framework.cpp
Lines 118-124 (patched)
<https://reviews.apache.org/r/61112/#comment257684>
For consistency, with other JSON flags, how about this:
```
"The value could be a JSON-formatted string of `URI`s that\n"
"should be fetched before running the executor, or a file\n"
"path containing the JSON-formatted `URI`s. Path must be of\n"
"the form `file:///path/to/file` or `/path/to/file`.\n"
"This flag replaces `--executor_uri`.\n"
"See the `CommandInfo::URI` message in `mesos.proto` for the\n"
"expected format.\n"
```
src/examples/balloon_framework.cpp
Lines 133-135 (original), 159-161 (patched)
<https://reviews.apache.org/r/61112/#comment257685>
Add a TODO for deprecation:
```
// TODO(armand): Remove the `--executor_uri` flag after the
// deprecation cycle, started in 1.4.0.
```
src/examples/balloon_framework.cpp
Line 500 (original), 533-534 (patched)
<https://reviews.apache.org/r/61112/#comment257686>
Blank line, please.
- Alexander Rukletsov
On Aug. 1, 2017, 10:20 a.m., Armand Grillet wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61112/
> -----------------------------------------------------------
>
> (Updated Aug. 1, 2017, 10:20 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
>
>
> Bugs: MESOS-7814
> https://issues.apache.org/jira/browse/MESOS-7814
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Allows to set URIs that will be fetched before running the executor.
>
>
> Diffs
> -----
>
> src/examples/balloon_framework.cpp b8c8e387bd8592bc1d27ff62fa9fd5397ad71fb9
> src/examples/long_lived_framework.cpp af5b43ee6994cf703f412436d6d561d9abd3b26d
>
>
> Diff: https://reviews.apache.org/r/61112/diff/3/
>
>
> Testing
> -------
>
> $ make check
>
>
> Thanks,
>
> Armand Grillet
>
>
Re: Review Request 61112: Added executor_uris flag to long lived and
balloon frameworks.
Posted by Armand Grillet <ag...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61112/
-----------------------------------------------------------
(Updated Aug. 1, 2017, 10:20 a.m.)
Review request for mesos, Alexander Rukletsov and Till Toenshoff.
Bugs: MESOS-7814
https://issues.apache.org/jira/browse/MESOS-7814
Repository: mesos
Description
-------
Allows to set URIs that will be fetched before running the executor.
Diffs
-----
src/examples/balloon_framework.cpp b8c8e387bd8592bc1d27ff62fa9fd5397ad71fb9
src/examples/long_lived_framework.cpp af5b43ee6994cf703f412436d6d561d9abd3b26d
Diff: https://reviews.apache.org/r/61112/diff/3/
Testing
-------
$ make check
Thanks,
Armand Grillet
Re: Review Request 61112: Added executor_uris flag to long lived and
balloon frameworks.
Posted by Armand Grillet <ag...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61112/
-----------------------------------------------------------
(Updated July 31, 2017, 9:39 p.m.)
Review request for mesos, Alexander Rukletsov and Till Toenshoff.
Changes
-------
Rsolved issues and flag added to long lived framework.
Summary (updated)
-----------------
Added executor_uris flag to long lived and balloon frameworks.
Bugs: MESOS-7814
https://issues.apache.org/jira/browse/MESOS-7814
Repository: mesos
Description
-------
Allows to set URIs that will be fetched before running the executor.
Diffs (updated)
-----
src/examples/balloon_framework.cpp b8c8e387bd8592bc1d27ff62fa9fd5397ad71fb9
src/examples/long_lived_framework.cpp af5b43ee6994cf703f412436d6d561d9abd3b26d
Diff: https://reviews.apache.org/r/61112/diff/3/
Changes: https://reviews.apache.org/r/61112/diff/2-3/
Testing
-------
$ make check
Thanks,
Armand Grillet
Re: Review Request 61112: Added uris flag to balloon framework.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61112/#review181799
-----------------------------------------------------------
src/examples/balloon_framework.cpp
Lines 20 (patched)
<https://reviews.apache.org/r/61112/#comment257552>
Do you still need this?
src/examples/balloon_framework.cpp
Lines 26 (patched)
<https://reviews.apache.org/r/61112/#comment257553>
Why do you need this one?
src/examples/balloon_framework.cpp
Lines 55 (patched)
<https://reviews.apache.org/r/61112/#comment257554>
Don't need this anymore?
src/examples/balloon_framework.cpp
Lines 127 (patched)
<https://reviews.apache.org/r/61112/#comment257548>
Let's call it `--executor_uris` for calrity.
src/examples/balloon_framework.cpp
Lines 132 (patched)
<https://reviews.apache.org/r/61112/#comment257555>
s\URI\CommandInfo::URI because `URI` is not a top level message.
src/examples/balloon_framework.cpp
Lines 498-504 (original), 529-551 (patched)
<https://reviews.apache.org/r/61112/#comment257549>
I'd suggest to use either `--executor_uri` or new `--uris` flag. Let's update comments for both flags to mention this, add a `CHECK` above that only a single one is set, and leave a deprecation `TODO` for the former flag.
src/examples/balloon_framework.cpp
Lines 540 (patched)
<https://reviews.apache.org/r/61112/#comment257550>
Indent is off.
src/examples/balloon_framework.cpp
Lines 543-545 (patched)
<https://reviews.apache.org/r/61112/#comment257556>
`EXIT(EXIT_FAILURE)` instead?
src/examples/balloon_framework.cpp
Lines 548-550 (patched)
<https://reviews.apache.org/r/61112/#comment257557>
Can we use `CopyFrom` from `RepeatedPtrField` here?
- Alexander Rukletsov
On July 31, 2017, 1:01 p.m., Armand Grillet wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61112/
> -----------------------------------------------------------
>
> (Updated July 31, 2017, 1:01 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Alexander Rojas.
>
>
> Bugs: MESOS-7814
> https://issues.apache.org/jira/browse/MESOS-7814
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Allows to set URIs that will be fetched before running the executor.
>
>
> Diffs
> -----
>
> src/examples/balloon_framework.cpp b8c8e387bd8592bc1d27ff62fa9fd5397ad71fb9
>
>
> Diff: https://reviews.apache.org/r/61112/diff/2/
>
>
> Testing
> -------
>
> $ make check
>
>
> Thanks,
>
> Armand Grillet
>
>
Re: Review Request 61112: Added uris flag to balloon framework.
Posted by Armand Grillet <ag...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61112/
-----------------------------------------------------------
(Updated July 31, 2017, 1:01 p.m.)
Review request for mesos, Alexander Rukletsov and Alexander Rojas.
Changes
-------
Suggestion applied.
Summary (updated)
-----------------
Added uris flag to balloon framework.
Bugs: MESOS-7814
https://issues.apache.org/jira/browse/MESOS-7814
Repository: mesos
Description (updated)
-------
Allows to set URIs that will be fetched before running the executor.
Diffs (updated)
-----
src/examples/balloon_framework.cpp b8c8e387bd8592bc1d27ff62fa9fd5397ad71fb9
Diff: https://reviews.apache.org/r/61112/diff/2/
Changes: https://reviews.apache.org/r/61112/diff/1-2/
Testing
-------
$ make check
Thanks,
Armand Grillet