You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by James Peach <jp...@apache.org> on 2017/01/06 01:13:52 UTC
Review Request 55239: Stop using os::system to extract fetcher
archives.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55239/
-----------------------------------------------------------
Review request for mesos, Jie Yu and Jiang Yan Xu.
Bugs: MESOS-6862
https://issues.apache.org/jira/browse/MESOS-6862
Repository: mesos
Description
-------
Stop using os::system to extract fetcher archives.
Diffs
-----
src/launcher/fetcher.cpp 4456c28139966e42859cc6d2c79a1d90e83fb22f
Diff: https://reviews.apache.org/r/55239/diff/
Testing
-------
`sudo make check` (Fedora 25)
Thanks,
James Peach
Re: Review Request 55239: Stop using os::system to extract fetcher
archives.
Posted by James Peach <jp...@apache.org>.
> On Jan. 11, 2017, 9:14 a.m., Jiang Yan Xu wrote:
> > I don't feel that we need to introduce these wrappers, which are either single-use or don't provide a better abstration than the counterpart in stout.
> >
> > Can we uniformly use `subprocess` in all cases (since `os::spawn()` doesn't work in all cases) for consistency?
> >
> > To reuse code, we can do this
> >
> > ```
> > vector<string> command;
> >
> > // Potentially need to specify destination via stdout.
> > Option<Subprocess::IO> out = None();
> >
> > // if tar.
> > command = {"tar", "-C", destinationDirectory, "-xf", sourcePath};
> > // else if gzip.
> > command = {"gzip", "-dc", sourcePath};
> > out = Subprocess::PATH(destinationPath);
> > // else if zip.
> > command = {"unzip", "-o", "-d", destinationDirectory, sourcePath};
> > // else
> >
> > ...
> >
> > // Comment that we use the argv of subprocess to avoid injection.
> > Try<Subprocess> s = subprocess(
> > command[0],
> > command,
> > Subprocess::FD(STDIN_FILENO),
> > out.getOrElse(Subprocess::FD(STDOUT_FILENO)));
> > ```
> >
> > What do you think?
We could subprocess everything. Would need to think about how to format the command message legibly.
> On Jan. 11, 2017, 9:14 a.m., Jiang Yan Xu wrote:
> > src/launcher/fetcher.cpp, line 85
> > <https://reviews.apache.org/r/55239/diff/1/?file=1598134#file1598134line85>
> >
> > This is the default so unnecessary?
However it makes it obvious to the reader, so it should be kept.
> On Jan. 11, 2017, 9:14 a.m., Jiang Yan Xu wrote:
> > src/launcher/fetcher.cpp, line 95
> > <https://reviews.apache.org/r/55239/diff/1/?file=1598134#file1598134line95>
> >
> > `stringify` on `vector` puts `", "` between items.
Yes, that's desirable so that you can see the whitespace in the command.
> On Jan. 11, 2017, 9:14 a.m., Jiang Yan Xu wrote:
> > src/launcher/fetcher.cpp, line 67
> > <https://reviews.apache.org/r/55239/diff/1/?file=1598134#file1598134line67>
> >
> > `stringify` on `vector` puts `", "` between items. We can use `strings::join()`.
This is intended since it makes the actual arguments much clearer and more obvious.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55239/#review161016
-----------------------------------------------------------
On Jan. 6, 2017, 1:13 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55239/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2017, 1:13 a.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stop using os::system to extract fetcher archives.
>
>
> Diffs
> -----
>
> src/launcher/fetcher.cpp 4456c28139966e42859cc6d2c79a1d90e83fb22f
>
> Diff: https://reviews.apache.org/r/55239/diff/
>
>
> Testing
> -------
>
> `sudo make check` (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 55239: Stop using os::system to extract fetcher
archives.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55239/#review161016
-----------------------------------------------------------
I don't feel that we need to introduce these wrappers, which are either single-use or don't provide a better abstration than the counterpart in stout.
Can we uniformly use `subprocess` in all cases (since `os::spawn()` doesn't work in all cases) for consistency?
To reuse code, we can do this
```
vector<string> command;
// Potentially need to specify destination via stdout.
Option<Subprocess::IO> out = None();
// if tar.
command = {"tar", "-C", destinationDirectory, "-xf", sourcePath};
// else if gzip.
command = {"gzip", "-dc", sourcePath};
out = Subprocess::PATH(destinationPath);
// else if zip.
command = {"unzip", "-o", "-d", destinationDirectory, sourcePath};
// else
...
// Comment that we use the argv of subprocess to avoid injection.
Try<Subprocess> s = subprocess(
command[0],
command,
Subprocess::FD(STDIN_FILENO),
out.getOrElse(Subprocess::FD(STDOUT_FILENO)));
```
What do you think?
src/launcher/fetcher.cpp (line 67)
<https://reviews.apache.org/r/55239/#comment232335>
`stringify` on `vector` puts `", "` between items. We can use `strings::join()`.
src/launcher/fetcher.cpp (line 85)
<https://reviews.apache.org/r/55239/#comment232293>
This is the default so unnecessary?
src/launcher/fetcher.cpp (line 95)
<https://reviews.apache.org/r/55239/#comment232295>
`stringify` on `vector` puts `", "` between items.
- Jiang Yan Xu
On Jan. 5, 2017, 5:13 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55239/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2017, 5:13 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stop using os::system to extract fetcher archives.
>
>
> Diffs
> -----
>
> src/launcher/fetcher.cpp 4456c28139966e42859cc6d2c79a1d90e83fb22f
>
> Diff: https://reviews.apache.org/r/55239/diff/
>
>
> Testing
> -------
>
> `sudo make check` (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 55239: Stop using os::system to extract fetcher
archives.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55239/
-----------------------------------------------------------
(Updated Jan. 17, 2017, 10:05 p.m.)
Review request for mesos, Jie Yu and Jiang Yan Xu.
Changes
-------
Rebase.
Bugs: MESOS-6862
https://issues.apache.org/jira/browse/MESOS-6862
Repository: mesos
Description
-------
Stop using os::system to extract fetcher archives.
Diffs (updated)
-----
src/launcher/fetcher.cpp 4456c28139966e42859cc6d2c79a1d90e83fb22f
Diff: https://reviews.apache.org/r/55239/diff/
Testing
-------
`sudo make check` (Fedora 25)
Thanks,
James Peach
Re: Review Request 55239: Stop using os::system to extract fetcher
archives.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55239/
-----------------------------------------------------------
(Updated Jan. 13, 2017, 11:11 p.m.)
Review request for mesos, Jie Yu and Jiang Yan Xu.
Changes
-------
Rebased.
Bugs: MESOS-6862
https://issues.apache.org/jira/browse/MESOS-6862
Repository: mesos
Description
-------
Stop using os::system to extract fetcher archives.
Diffs (updated)
-----
src/launcher/fetcher.cpp 4456c28139966e42859cc6d2c79a1d90e83fb22f
Diff: https://reviews.apache.org/r/55239/diff/
Testing
-------
`sudo make check` (Fedora 25)
Thanks,
James Peach
Re: Review Request 55239: Stop using os::system to extract fetcher
archives.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55239/
-----------------------------------------------------------
(Updated Jan. 12, 2017, 9:47 p.m.)
Review request for mesos, Jie Yu and Jiang Yan Xu.
Changes
-------
Addressed review feedback.
Bugs: MESOS-6862
https://issues.apache.org/jira/browse/MESOS-6862
Repository: mesos
Description
-------
Stop using os::system to extract fetcher archives.
Diffs (updated)
-----
src/launcher/fetcher.cpp 4456c28139966e42859cc6d2c79a1d90e83fb22f
Diff: https://reviews.apache.org/r/55239/diff/
Testing
-------
`sudo make check` (Fedora 25)
Thanks,
James Peach
Re: Review Request 55239: Stop using os::system to extract fetcher
archives.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On Jan. 12, 2017, 12:36 p.m., Jiang Yan Xu wrote:
> > src/launcher/fetcher.cpp, line 105
> > <https://reviews.apache.org/r/55239/diff/2/?file=1603239#file1603239line105>
> >
> > Quote the command as well?
What about this?
> On Jan. 12, 2017, 12:36 p.m., Jiang Yan Xu wrote:
> > src/launcher/fetcher.cpp, line 113
> > <https://reviews.apache.org/r/55239/diff/2/?file=1603239#file1603239line113>
> >
> > Quote the command as well?
What about this?
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55239/#review161409
-----------------------------------------------------------
On Jan. 13, 2017, 3:11 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55239/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2017, 3:11 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stop using os::system to extract fetcher archives.
>
>
> Diffs
> -----
>
> src/launcher/fetcher.cpp 4456c28139966e42859cc6d2c79a1d90e83fb22f
>
> Diff: https://reviews.apache.org/r/55239/diff/
>
>
> Testing
> -------
>
> `sudo make check` (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 55239: Stop using os::system to extract fetcher
archives.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55239/#review161409
-----------------------------------------------------------
Fix it, then Ship it!
src/launcher/fetcher.cpp (line 51)
<https://reviews.apache.org/r/55239/#comment232650>
#include <vector>?
src/launcher/fetcher.cpp (line 87)
<https://reviews.apache.org/r/55239/#comment232651>
This is fine too but apparently the input doesn't need to come from stdin, it can just be an argument.
src/launcher/fetcher.cpp (line 96)
<https://reviews.apache.org/r/55239/#comment232655>
This is me nit-picking but as a general good practice, put a `CHECK_GT(command.size(), 0u)` above so it's future proof (that `command` won't go through some branch that doesn't set it)?
src/launcher/fetcher.cpp (line 105)
<https://reviews.apache.org/r/55239/#comment232653>
Quote the command as well?
src/launcher/fetcher.cpp (line 109)
<https://reviews.apache.org/r/55239/#comment232652>
Just add a simple comment:
```
// `status()` never fails or gets discarded.
```
because this is not obvious. I'll send a patch to better document this.
src/launcher/fetcher.cpp (line 113)
<https://reviews.apache.org/r/55239/#comment232654>
Quote the command as well?
- Jiang Yan Xu
On Jan. 11, 2017, 4:27 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55239/
> -----------------------------------------------------------
>
> (Updated Jan. 11, 2017, 4:27 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stop using os::system to extract fetcher archives.
>
>
> Diffs
> -----
>
> src/launcher/fetcher.cpp 4456c28139966e42859cc6d2c79a1d90e83fb22f
>
> Diff: https://reviews.apache.org/r/55239/diff/
>
>
> Testing
> -------
>
> `sudo make check` (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 55239: Stop using os::system to extract fetcher
archives.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55239/
-----------------------------------------------------------
(Updated Jan. 12, 2017, 12:27 a.m.)
Review request for mesos, Jie Yu and Jiang Yan Xu.
Changes
-------
Rebase and address review feedback.
Bugs: MESOS-6862
https://issues.apache.org/jira/browse/MESOS-6862
Repository: mesos
Description
-------
Stop using os::system to extract fetcher archives.
Diffs (updated)
-----
src/launcher/fetcher.cpp 4456c28139966e42859cc6d2c79a1d90e83fb22f
Diff: https://reviews.apache.org/r/55239/diff/
Testing
-------
`sudo make check` (Fedora 25)
Thanks,
James Peach
Re: Review Request 55239: Stop using os::system to extract fetcher
archives.
Posted by James Peach <jp...@apache.org>.
> On Jan. 11, 2017, 9:55 a.m., Jiang Yan Xu wrote:
> > src/launcher/fetcher.cpp, line 91
> > <https://reviews.apache.org/r/55239/diff/1/?file=1598134#file1598134line91>
> >
> > `status()` is not guaranteed to be ready, this could crash the fetcher process.
This implicitly awaits the future. It would crash if the future failed or is discarded, which AFAIK can't happen here.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55239/#review161197
-----------------------------------------------------------
On Jan. 6, 2017, 1:13 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55239/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2017, 1:13 a.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stop using os::system to extract fetcher archives.
>
>
> Diffs
> -----
>
> src/launcher/fetcher.cpp 4456c28139966e42859cc6d2c79a1d90e83fb22f
>
> Diff: https://reviews.apache.org/r/55239/diff/
>
>
> Testing
> -------
>
> `sudo make check` (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 55239: Stop using os::system to extract fetcher
archives.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55239/#review161197
-----------------------------------------------------------
src/launcher/fetcher.cpp (line 91)
<https://reviews.apache.org/r/55239/#comment232436>
`status()` is not guaranteed to be ready, this could crash the fetcher process.
- Jiang Yan Xu
On Jan. 5, 2017, 5:13 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55239/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2017, 5:13 p.m.)
>
>
> Review request for mesos, Jie Yu and Jiang Yan Xu.
>
>
> Bugs: MESOS-6862
> https://issues.apache.org/jira/browse/MESOS-6862
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stop using os::system to extract fetcher archives.
>
>
> Diffs
> -----
>
> src/launcher/fetcher.cpp 4456c28139966e42859cc6d2c79a1d90e83fb22f
>
> Diff: https://reviews.apache.org/r/55239/diff/
>
>
> Testing
> -------
>
> `sudo make check` (Fedora 25)
>
>
> Thanks,
>
> James Peach
>
>