You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Artem Harutyunyan <ar...@mesosphere.io> on 2015/08/05 04:54:19 UTC
Re: Review Request 36978: MESOS-3142 Refactoring os::shell - patch 1/2
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36978/#review94162
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp (line 40)
<https://reviews.apache.org/r/36978/#comment148715>
Was not it discussed at some point that it would be good to have a funtion that returns a {status, stderr, stdout} tuple?
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp (line 50)
<https://reviews.apache.org/r/36978/#comment148697>
should the variable be called `_cmd`?
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp (line 57)
<https://reviews.apache.org/r/36978/#comment148703>
Why not do `std::string cmd = _cmd + " " + strings::join(" ", args)?` and get rid of cmdLine?
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp (lines 86 - 88)
<https://reviews.apache.org/r/36978/#comment148705>
alignment seems to be off here.
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp (line 87)
<https://reviews.apache.org/r/36978/#comment148708>
Is there are reason for specifically calling out error 127? I would suggest a generic error message + stringify(WEXITSTATUS(status)).
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp (line 881)
<https://reviews.apache.org/r/36978/#comment148714>
Maybe it makes sense to factor out os::shell tests into a separate function.
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp (line 882)
<https://reviews.apache.org/r/36978/#comment148713>
Could you please add a test that makes sure that redirecting stderr to stdout with 2>&1 works?
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp (line 885)
<https://reviews.apache.org/r/36978/#comment148711>
From what I've seen the variables with _ are used when some transformation needs to be applied on the original value:
```
_var = transform1(__var);
var = transform2(_var);
```
- Artem Harutyunyan
On Aug. 4, 2015, 5:54 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36978/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2015, 5:54 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
>
>
> Bugs: MESOS-3142
> https://issues.apache.org/jira/browse/MESOS-3142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactoring os::shell.
> See MESOS-3142 for more details.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp ab767ccc4553cc5f61e4fe1b67110a9b5b32f2bc
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp 53f14e1869ed7a6e1ac7cc8a82c558ed77907dc9
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 2556bd428cc8990659e30e804b9c96c1659ef4a1
>
> Diff: https://reviews.apache.org/r/36978/diff/
>
>
> Testing
> -------
>
> make check
> *Note*: this patch by itself breaks mesos - this only fixes the `stout` part: see also https://reviews.apache.org/r/36979/
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 36978: MESOS-3142 Refactoring os::shell - patch 1/2
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On Aug. 5, 2015, 2:54 a.m., Artem Harutyunyan wrote:
> >
Thanks for super-quick review!
> On Aug. 5, 2015, 2:54 a.m., Artem Harutyunyan wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp, line 40
> > <https://reviews.apache.org/r/36978/diff/1/?file=1026032#file1026032line40>
> >
> > Was not it discussed at some point that it would be good to have a funtion that returns a {status, stderr, stdout} tuple?
This was relative to upgrading `process::Subprocess` - the idea here was the opposite: to greatly simplify the interface (and usage pattern) for `os::shell`
See https://reviews.apache.org/r/36424/
> On Aug. 5, 2015, 2:54 a.m., Artem Harutyunyan wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp, line 50
> > <https://reviews.apache.org/r/36978/diff/1/?file=1026032#file1026032line50>
> >
> > should the variable be called `_cmd`?
note this is neither a private member, not a constructor arg - it's a temp var: AFAIK there are no guidelines (apart from the obvious naming it sensibly).
Renamed `command`
> On Aug. 5, 2015, 2:54 a.m., Artem Harutyunyan wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp, lines 97-99
> > <https://reviews.apache.org/r/36978/diff/1/?file=1026032#file1026032line97>
> >
> > alignment seems to be off here.
uh? how did *that* happen?
thanks!
> On Aug. 5, 2015, 2:54 a.m., Artem Harutyunyan wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp, line 66
> > <https://reviews.apache.org/r/36978/diff/1/?file=1026032#file1026032line66>
> >
> > Why not do `std::string cmd = _cmd + " " + strings::join(" ", args)?` and get rid of cmdLine?
why not, indeed!
> On Aug. 5, 2015, 2:54 a.m., Artem Harutyunyan wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp, line 98
> > <https://reviews.apache.org/r/36978/diff/1/?file=1026032#file1026032line98>
> >
> > Is there are reason for specifically calling out error 127? I would suggest a generic error message + stringify(WEXITSTATUS(status)).
Because I'm trying to be a nice guy? :)
Error 127 is really usually a "command not found" code (at least in *nix systems - no idea about Win32)
> On Aug. 5, 2015, 2:54 a.m., Artem Harutyunyan wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp, line 881
> > <https://reviews.apache.org/r/36978/diff/1/?file=1026033#file1026033line881>
> >
> > Maybe it makes sense to factor out os::shell tests into a separate function.
I have created a new test fixture for os::shell specifically; adding a new file would seem a bit overkill?
(note the other review has a lot of tests for os::shell updated too, so refactoring would be a bit of a major effort for not much gain).
> On Aug. 5, 2015, 2:54 a.m., Artem Harutyunyan wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp, line 885
> > <https://reviews.apache.org/r/36978/diff/1/?file=1026033#file1026033line885>
> >
> > From what I've seen the variables with _ are used when some transformation needs to be applied on the original value:
> >
> > ```
> > _var = transform1(__var);
> > var = transform2(_var);
> > ```
I'm just reusing what was already there?
The rationale, I guess, was to name it in an obvious way, but leaving room for the actual `user` further down? I don't know, really.
Do we really care? usage/intent seems abundantly clear here.
> On Aug. 5, 2015, 2:54 a.m., Artem Harutyunyan wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp, line 882
> > <https://reviews.apache.org/r/36978/diff/1/?file=1026033#file1026033line882>
> >
> > Could you please add a test that makes sure that redirecting stderr to stdout with 2>&1 works?
Awesome catch!
turns out that even adding `2>&1` was going to be ignored, as the non-zero exit code forces an `Error` to be returned.
I've added an `ignoreErrors` flag, that could be used for this purpose:
```
result = os::shell("ls /tmp/foobar889076 2>&1", vector<string>{}, true);
```
see the tests for an example.
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36978/#review94162
-----------------------------------------------------------
On Aug. 5, 2015, 12:54 a.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36978/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2015, 12:54 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
>
>
> Bugs: MESOS-3142
> https://issues.apache.org/jira/browse/MESOS-3142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactoring os::shell.
> See MESOS-3142 for more details.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp ab767ccc4553cc5f61e4fe1b67110a9b5b32f2bc
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp 53f14e1869ed7a6e1ac7cc8a82c558ed77907dc9
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 2556bd428cc8990659e30e804b9c96c1659ef4a1
>
> Diff: https://reviews.apache.org/r/36978/diff/
>
>
> Testing
> -------
>
> make check
> *Note*: this patch by itself breaks mesos - this only fixes the `stout` part: see also https://reviews.apache.org/r/36979/
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 36978: MESOS-3142 Refactoring os::shell - patch 1/2
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On Aug. 5, 2015, 2:54 a.m., Artem Harutyunyan wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp, line 50
> > <https://reviews.apache.org/r/36978/diff/1/?file=1026032#file1026032line50>
> >
> > should the variable be called `_cmd`?
>
> Marco Massenzio wrote:
> note this is neither a private member, not a constructor arg - it's a temp var: AFAIK there are no guidelines (apart from the obvious naming it sensibly).
> Renamed `command`
>
> Artem Harutyunyan wrote:
> It does not really matter since you renamed, but there is a guideline for function arguments:
>
> ```
> We prepend constructor and function arguments with a leading underscore to avoid ambiguity and / or shadowing:
> ```
ah!
It *does* matter: I know something that I didn't, but should have :)
Thanks!
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36978/#review94162
-----------------------------------------------------------
On Aug. 6, 2015, 6:20 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36978/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2015, 6:20 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
>
>
> Bugs: MESOS-3142
> https://issues.apache.org/jira/browse/MESOS-3142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactoring os::shell.
> See MESOS-3142 for more details.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp ab767ccc4553cc5f61e4fe1b67110a9b5b32f2bc
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp 53f14e1869ed7a6e1ac7cc8a82c558ed77907dc9
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp 4b7a7bafe1c64183d021b39cf12523250491f0ee
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 2556bd428cc8990659e30e804b9c96c1659ef4a1
>
> Diff: https://reviews.apache.org/r/36978/diff/
>
>
> Testing
> -------
>
> make check
> *Note*: this patch by itself breaks mesos - this only fixes the `stout` part: see also https://reviews.apache.org/r/36979/
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 36978: MESOS-3142 Refactoring os::shell - patch 1/2
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
> On Aug. 4, 2015, 7:54 p.m., Artem Harutyunyan wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp, line 50
> > <https://reviews.apache.org/r/36978/diff/1/?file=1026032#file1026032line50>
> >
> > should the variable be called `_cmd`?
>
> Marco Massenzio wrote:
> note this is neither a private member, not a constructor arg - it's a temp var: AFAIK there are no guidelines (apart from the obvious naming it sensibly).
> Renamed `command`
It does not really matter since you renamed, but there is a guideline for function arguments:
```
We prepend constructor and function arguments with a leading underscore to avoid ambiguity and / or shadowing:
```
- Artem
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36978/#review94162
-----------------------------------------------------------
On Aug. 6, 2015, 11:20 a.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36978/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2015, 11:20 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
>
>
> Bugs: MESOS-3142
> https://issues.apache.org/jira/browse/MESOS-3142
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactoring os::shell.
> See MESOS-3142 for more details.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp ab767ccc4553cc5f61e4fe1b67110a9b5b32f2bc
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp 53f14e1869ed7a6e1ac7cc8a82c558ed77907dc9
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp 4b7a7bafe1c64183d021b39cf12523250491f0ee
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 2556bd428cc8990659e30e804b9c96c1659ef4a1
>
> Diff: https://reviews.apache.org/r/36978/diff/
>
>
> Testing
> -------
>
> make check
> *Note*: this patch by itself breaks mesos - this only fixes the `stout` part: see also https://reviews.apache.org/r/36979/
>
>
> Thanks,
>
> Marco Massenzio
>
>