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 Wed, 05 Aug 2015 17:08:58 GMT


> On Aug. 5, 2015, 4:16 a.m., Artem Harutyunyan wrote:
> > src/tests/containerizer/isolator_tests.cpp, line 1269
> > <https://reviews.apache.org/r/36979/diff/1/?file=1026037#file1026037line1269>
> >
> >     You are right that the awk did not actually seem to accomplish anything meaningful
here.

my major concern is that these are ROOT Container tests that won't be run on OSX (and won't
be run often either) - so wanted to mark it, as if the test fails we know who to blame (me!)

I'll double check on an Ubuntu server too.


> On Aug. 5, 2015, 4:16 a.m., Artem Harutyunyan wrote:
> > src/tests/containerizer/port_mapping_tests.cpp, lines 971-974
> > <https://reviews.apache.org/r/36979/diff/1/?file=1026038#file1026038line971>
> >
> >     Another illustration of why a tuple return type might be a better option for
os::shell() :-)
> >     
> >     But regardless, I'd change this code to something more suggestive (it's a test
case after all), or at least would add a comment that clarifies the intention.

Added a comment, that was sorely needed, you're right!
As for the tuple, that's what `process::Subprocess` will be for.
We assume that `os::shell` usage is when one wants to just run a command and only cares: did
it succeed?
(this was actually the **only** place in the code base where anyone cared about the exit code,
believe it or not).


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

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?


- Marco


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


On Aug. 5, 2015, 12:55 a.m., Marco Massenzio wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36979/
> -----------------------------------------------------------
> 
> (Updated Aug. 5, 2015, 12:55 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
> -------
> 
> 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 3f6e9df8711995d0dd3903c6170fdd5ad61aac5a

>   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