Re: Review Request 36979: Updating all references to os::shell
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 21 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
Re: Review Request 36979: Updating all references to os::shell
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)); Also, do these need to be ASSERT? I know you're just inheriting bad code here, but might as well clean it up. - Benjamin --- 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
Re: Review Request 36979: Updating all references to os::shell
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36979/ --- (Updated Aug. 11, 2015, 7:37 a.m.) Review request for mesos, Benjamin Hindman and Artem Harutyunyan. Changes --- Ben's comments. 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 (updated) - src/hdfs/hdfs.hpp a070c3200f0a0ac48ec86451749c7faf10c7f6a7 src/master/main.cpp e05a472b86170eb26df26aaa4b65437fcdd413ce src/slave/containerizer/isolators/network/port_mapping.cpp 88c0cbc61f3f97b084cc3b3fae8999b07d4aa1c7 src/tests/containerizer/isolator_tests.cpp dd1ae22865ce4467da5ed819e7f0a1cbb834371d src/tests/containerizer/port_mapping_tests.cpp 3c9b7c816a03e2994a353674c5963f1dda043124 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
Re: Review Request 36979: Updating all references to os::shell
--- 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
Re: Review Request 36979: Updating all references to os::shell
On Aug. 6, 2015, 10:26 p.m., Guangya Liu wrote: src/hdfs/hdfs.hpp, line 110 https://reviews.apache.org/r/36979/diff/3/?file=1033647#file1033647line110 Why not use the following? return !out.get().empty(); Actually, that was exactly what I originally typed :) However, if you see the original code, the semantic is subtly different: it only checked for a non-zero exit code (which would be now guaranteed by the `! isError()`). So, changing as you suggest (which, incidentally, I agree on) would in fact require that the `version` command emits something non-trivial to stdout: as the method is sadly not commented, I don't know whether changing that would break existing code. I'll wait to hear what @BenH (or someone else more knowledgeable than me on this matter, at any rate) has to say. - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36979/#review94453 --- 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
Re: Review Request 36979: Updating all references to os::shell
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36979/#review94452 --- Patch looks great! Reviews applied: [36978, 36979] All tests passed. - Mesos ReviewBot 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
Re: Review Request 36979: Updating all references to os::shell
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36979/#review94453 --- src/hdfs/hdfs.hpp (line 110) https://reviews.apache.org/r/36979/#comment149063 Why not use the following? return !out.get().empty(); - Guangya Liu On 八月 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 八月 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
Re: Review Request 36979: Updating all references to os::shell
--- 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. Changes --- Updated call signatures. 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 (updated) - 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
Re: Review Request 36979: Updating all references to os::shell
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. Marco Massenzio wrote: 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? Artem Harutyunyan wrote: After think a bit more about it, I find it a bit unusual that the user has to perform a string search in order to get out the integer error code. In cases when you expect a certain kind of failure from a certain command it's easy (like in your test case), but what if the cause of failure is unknown, or if there are several possible error codes expected. It looks to me that one will need to involve a regex parser to be able to reliably(?) get the signal and the error code out. This might drive delopers away from this function, and cause proliferation of similar code in the codebase (something that this was meant to facilitate avoiding). Returning a primtive struct(or a union) with a couple of fields could easily help to avoid that. Marco Massenzio wrote: Well, as a data point, as mentioned, the use case you mention, was **never** used in our current (what, 2-3year old?) code base. And, in any event, if anyone wants a rich outcome, they can use `process::subprocess` (which I'm updating next) and they'll get pretty much everything they ever wanted to know about the outcome. This is really meant to be an easy-to-use, low-boilerplate API for the simple use case: I want to run `cmd` and only care whether it succeeded or not. see `CommandResult` in https://reviews.apache.org/r/36424/diff/6#index_header - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36979/#review94177 --- 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
Re: Review Request 36979: Updating all references to os::shell
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. Marco Massenzio wrote: 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? Artem Harutyunyan wrote: After think a bit more about it, I find it a bit unusual that the user has to perform a string search in order to get out the integer error code. In cases when you expect a certain kind of failure from a certain command it's easy (like in your test case), but what if the cause of failure is unknown, or if there are several possible error codes expected. It looks to me that one will need to involve a regex parser to be able to reliably(?) get the signal and the error code out. This might drive delopers away from this function, and cause proliferation of similar code in the codebase (something that this was meant to facilitate avoiding). Returning a primtive struct(or a union) with a couple of fields could easily help to avoid that. Well, as a data point, as mentioned, the use case you mention, was **never** used in our current (what, 2-3year old?) code base. And, in any event, if anyone wants a rich outcome, they can use `process::subprocess` (which I'm updating next) and they'll get pretty much everything they ever wanted to know about the outcome. This is really meant to be an easy-to-use, low-boilerplate API for the simple use case: I want to run `cmd` and only care whether it succeeded or not. - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36979/#review94177 --- 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
Re: Review Request 36979: Updating all references to os::shell
On Aug. 4, 2015, 9:16 p.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. Marco Massenzio wrote: 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? After think a bit more about it, I find it a bit unusual that the user has to perform a string search in order to get out the integer error code. In cases when you expect a certain kind of failure from a certain command it's easy (like in your test case), but what if the cause of failure is unknown, or if there are several possible error codes expected. It looks to me that one will need to involve a regex parser to be able to reliably(?) get the signal and the error code out. This might drive delopers away from this function, and cause proliferation of similar code in the codebase (something that this was meant to facilitate avoiding). Returning a primtive struct(or a union) with a couple of fields could easily help to avoid that. - Artem --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36979/#review94177 --- On Aug. 5, 2015, 10:10 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, 10:10 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 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
Re: Review Request 36979: Updating all references to os::shell
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
Re: Review Request 36979: Updating all references to os::shell
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36979/ --- (Updated Aug. 5, 2015, 5:10 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 (updated) - 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
Re: Review Request 36979: Updating all references to os::shell
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36979/#review94286 --- Patch looks great! Reviews applied: [36978, 36979] All tests passed. - Mesos ReviewBot On Aug. 5, 2015, 5:10 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36979/ --- (Updated Aug. 5, 2015, 5:10 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
Re: Review Request 36979: Updating all references to os::shell
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36979/#review94177 --- src/slave/containerizer/isolators/network/port_mapping.cpp (line 1579) https://reviews.apache.org/r/36979/#comment148718 nit: this seems to fit on a single line. src/tests/containerizer/isolator_tests.cpp (line 1269) https://reviews.apache.org/r/36979/#comment148719 You are right that the awk did not actually seem to accomplish anything meaningful here. src/tests/containerizer/port_mapping_tests.cpp (lines 971 - 974) https://reviews.apache.org/r/36979/#comment148720 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. src/tests/containerizer/port_mapping_tests.cpp (line 986) https://reviews.apache.org/r/36979/#comment148721 ditto. + extra newline. - Artem Harutyunyan On Aug. 4, 2015, 5:55 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36979/ --- (Updated Aug. 4, 2015, 5:55 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 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
Review Request 36979: Updating all references to os::shell
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36979/ --- Review request for mesos. 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
Re: Review Request 36979: Updating all references to os::shell
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36979/#review93709 --- Patch looks great! Reviews applied: [36978, 36979] All tests passed. - Mesos ReviewBot On July 31, 2015, 8:04 a.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36979/ --- (Updated July 31, 2015, 8:04 a.m.) Review request for mesos. 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
