You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Artem Harutyunyan <ar...@mesosphere.io> on 2015/08/05 06:16:31 UTC
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
>
>
Re: Review Request 36979: Updating all references to os::shell
Posted by Marco Massenzio <ma...@mesosphere.io>.
> 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
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
> 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
Posted by Marco Massenzio <ma...@mesosphere.io>.
> 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
Posted by Marco Massenzio <ma...@mesosphere.io>.
> 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
>
>