mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Hindman" <b...@berkeley.edu>
Subject Re: Review Request 36979: Updating all references to os::shell
Date Tue, 11 Aug 2015 06:00:51 GMT

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

Ship it!



src/hdfs/hdfs.hpp (lines 69 - 88)
<https://reviews.apache.org/r/36979/#comment149549>

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



src/hdfs/hdfs.hpp (line 80)
<https://reviews.apache.org/r/36979/#comment149550>

    Not your bug but do you mind s/if(/if (/ please, thanks!



src/hdfs/hdfs.hpp (lines 103 - 105)
<https://reviews.apache.org/r/36979/#comment149551>

    Instead of this comment I think you could add a comment that explains why we want the
output for debugging!



src/hdfs/hdfs.hpp (lines 123 - 127)
<https://reviews.apache.org/r/36979/#comment149552>

    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?



src/hdfs/hdfs.hpp (line 159)
<https://reviews.apache.org/r/36979/#comment149553>

    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.



src/tests/containerizer/port_mapping_tests.cpp (line 975)
<https://reviews.apache.org/r/36979/#comment149554>

    Minor nit, how about here and below:
    
    ASSERT_FALSE(strings::contains(invalid.error(), "256"));


- Benjamin Hindman


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