mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marco Massenzio" <ma...@mesosphere.io>
Subject Re: Review Request 36978: MESOS-3142 Refactoring os::shell - patch 1/2
Date Wed, 05 Aug 2015 07:27:05 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message