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

Reply via email to