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 Tue, 11 Aug 2015 07:17:51 GMT


> On Aug. 11, 2015, 6 a.m., Benjamin Hindman wrote:
> > src/hdfs/hdfs.hpp, lines 123-127
> > <https://reviews.apache.org/r/36979/diff/3/?file=1033647#file1033647line123>
> >
> >     Wait, how was `|| true` the existing semantics? We are definitely capturing
stderr into stdout, but I don't see anything else here unless I'm missing something?

You are actually right - adding "|| true" alters the semantics - removed.

However, please note that, as we discussed, once the shell command exits with a non-zero error
code, we just Error() out, and do not return stdout (or stderr, for that matter).
The error message (stderr piped to stdout) will be in the logs emitted by `os::shell()`


> On Aug. 11, 2015, 6 a.m., Benjamin Hindman wrote:
> > src/hdfs/hdfs.hpp, line 159
> > <https://reviews.apache.org/r/36979/diff/3/?file=1033647#file1033647line159>
> >
> >     This is a really pesky nit, but in the above functions you decided to call the
variable capturing the result of `os::shell` to be called `out`, but here and below you decided
`output`? Let's pick one and be consistent per this file please.

yes, sorry, I was re-using existing variables as far as I could and, as they are local variables,
I didn't consider that.
Done.


> On Aug. 11, 2015, 6 a.m., Benjamin Hindman wrote:
> > src/hdfs/hdfs.hpp, lines 69-88
> > <https://reviews.apache.org/r/36979/diff/3/?file=1033647#file1033647line69>
> >
> >     Let's actually capture the error message because when debugging it'll be super
nice to see why it failed in the logs instead of nothing (so kill the "simplification" comment
and if you're concerned that someone will just come around and simplify it later leave a comment
why we're capturing `out.error()`.
> >     
> >     Then let's just return `true` if not an error to keep with previous semantics
(I don't know the details of `hadoop version` to suggest otherwise, so no need to stray).

LOL - so I did some further analysis and the original code actually was NOT doing what it
thought it did (I think - difficult to say: no docs).
By passing NULL as the &out in os::shell() it was not getting anything: looking at shell.hpp#L56
(`if (os != NULL) { ... }`) - adding the 2>&1 was a nice touch :)

Anyways, I did as you suggested, but I'm afraid the error message won't be that much helpful
(apart from stating that `hadoop version` failed with exit code xx).
BUT, I quite like your suggestion, so I've added a `LOG(ERROR) << stdout` in os::shell()
if the exit code != 0.
(please let me know if that's overkill, though!)


> On Aug. 11, 2015, 6 a.m., Benjamin Hindman wrote:
> > src/tests/containerizer/port_mapping_tests.cpp, line 975
> > <https://reviews.apache.org/r/36979/diff/3/?file=1033651#file1033651line975>
> >
> >     Minor nit, how about here and below:
> >     
> >     ASSERT_FALSE(strings::contains(invalid.error(), "256"));
> 
> Benjamin Hindman wrote:
>     Also, do these need to be ASSERT? I know you're just inheriting bad code here, but
might as well clean it up.

Done (and it was ASSERT_TRUE() but I got the point :)


- Marco


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


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