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