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 36979: Updating all references to os::shell
Date Thu, 06 Aug 2015 19:27:04 GMT


> On Aug. 5, 2015, 4:16 a.m., Artem Harutyunyan wrote:
> > src/tests/containerizer/port_mapping_tests.cpp, line 986
> > <https://reviews.apache.org/r/36979/diff/1/?file=1026038#file1026038line986>
> >
> >     ditto. 
> >     + extra newline.
> 
> Marco Massenzio wrote:
>     Having looked at both tests, I was being unnecessarily pedantic IMO: checking for
the error code (256) to be present in the error string seems to me to be more than sufficient
(and self-explanatory too - but added a comment all the same).
>     
>     What thinks you?
> 
> Artem Harutyunyan wrote:
>     After think a bit more about it, I find it a bit unusual that the user has to perform
a string search in order to get out the integer error code. In cases when you expect a certain
kind of failure from a certain command it's easy (like in your test case), but what if the
cause of failure is unknown, or if there are several possible error codes expected. It looks
to me that one will need to involve a regex parser to be able to reliably(?) get the signal
and the error code out. This might drive delopers away from this function, and cause proliferation
of similar code in the codebase (something that this was meant to facilitate avoiding). Returning
a primtive struct(or a union) with a couple of fields could easily help to avoid that.
> 
> Marco Massenzio wrote:
>     Well, as a data point, as mentioned, the use case you mention, was **never** used
in our current (what, 2-3year old?) code base.
>     And, in any event, if anyone wants a "rich" outcome, they can use `process::subprocess`
(which I'm updating next) and they'll get pretty much everything they ever wanted to know
about the outcome.
>     
>     This is really meant to be an easy-to-use, low-boilerplate API for the simple use
case: I want to run `cmd` and only care whether it succeeded or not.

see `CommandResult` in https://reviews.apache.org/r/36424/diff/6#index_header


- Marco


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36979/#review94177
-----------------------------------------------------------


On Aug. 6, 2015, 6:24 p.m., Marco Massenzio wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36979/
> -----------------------------------------------------------
> 
> (Updated Aug. 6, 2015, 6:24 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
> -------
> 
> Updating all references to os::shell
> For more details see MESOS-3142.
> 
> 
> Diffs
> -----
> 
>   src/hdfs/hdfs.hpp a070c3200f0a0ac48ec86451749c7faf10c7f6a7 
>   src/master/main.cpp e05a472b86170eb26df26aaa4b65437fcdd413ce 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 8244c345b84108af7fa18d20e71401d6e1a0aeb0

>   src/tests/containerizer/isolator_tests.cpp ff6e2b7e190a58a4809d6e71addb15dabe418e17

>   src/tests/containerizer/port_mapping_tests.cpp 4bee74acba2b1472c80cabbc9d0384bd04c543aa

> 
> Diff: https://reviews.apache.org/r/36979/diff/
> 
> 
> Testing
> -------
> 
> make check
> *Note*: this patch fixes breakages introduce by the refactoring in: https://reviews.apache.org/r/36978
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>


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