You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Marco Massenzio <ma...@mesosphere.io> on 2015/07/14 06:21:14 UTC
Re: Review Request 36424: Created a command executor helper method.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36424/
-----------------------------------------------------------
(Updated July 14, 2015, 4:21 a.m.)
Review request for mesos, Benjamin Hindman and Cody Maloney.
Bugs: MESOS-3035
https://issues.apache.org/jira/browse/MESOS-3035
Repository: mesos
Description
-------
Jira: MESOS-2902
While researching how to execute an arbitrary script
to detect the Master IP address, it emerged clearly that
a helper method to execute an arbitrary command/script on
a node and obtain either stdout or stderr would have been
useful and avoided a lot of code repetition.
This could not be ultimately used for the purpose at hand,
but I believe it to be useful enough (particularly, to avoid
people doing "coding by copy&paste" and/or waste time
researching the same functionality).
This would also be beneficial in MESOS-2830, factoring out the remote command execution logic.
Diffs (updated)
-----
3rdparty/libprocess/include/process/subprocess.hpp 310cb4f8e4e2faa5545dffd196d7490c868bc5d6
3rdparty/libprocess/src/tests/subprocess_tests.cpp f6acb204582a9e696c3b09d4e4c543bb052e97d4
Diff: https://reviews.apache.org/r/36424/diff/
Testing
-------
make check
Thanks,
Marco Massenzio
Re: Review Request 36424: Created a command executor helper method.
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On July 17, 2015, 8:48 a.m., Alexander Rojas wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 302-304
> > <https://reviews.apache.org/r/36424/diff/2/?file=1010778#file1010778line302>
> >
> > You may ignore this, but I'm not sure if ignoring `cerr` when the command succeds is the way to go. Most logging systems log unto the `clog` which is mostly an alias for the `cerr` file descriptor.
not ignoring this at all :)
In fact, we've agreed to return a `struct` with the composite info (retcode, stdout, stderr) - as you are absolutely right, sometimes even successful execution emit useful logs on stderr.
> On July 17, 2015, 8:48 a.m., Alexander Rojas wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 307
> > <https://reviews.apache.org/r/36424/diff/2/?file=1010778#file1010778line307>
> >
> > I think you should calle it `execute` to follow our concise naming. Moreover, under the context is hard to misunderstand it.
You are correct; however, my concern is that the verb `execute` has been vastly overloaded in the Mesos codebase, as well as being prone (here) to misunderstanding.
I would prefer to keep it as is, but I'm happy to change it, if there is consensus that calling it `execute` won't cause confusion.
> On July 17, 2015, 8:48 a.m., Alexander Rojas wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, line 314
> > <https://reviews.apache.org/r/36424/diff/2/?file=1010778#file1010778line314>
> >
> > ranged based for loops are not yet whitelisted, nor there are any occurrence on the codebase. So I guess it is not yet time to use them.
Well, I am a vocal critic of "whitelisting" - in theory, this may mean that our style guide could potentially become as large as the ISO standard guide? :)
I don't really see any value in *not* using this here: it's totally clear, it's supported by **all** the compilers/platforms we support and there is no chance of this being misinterpreted.
I don't see what continuing to use something that has been, honestly, superseded 4 years ago by the C++11 standard would buy us?
Also, I note in passing that your argument is [tautologic](https://en.wikipedia.org/wiki/Circular_reasoning): we should not use X because we have no occurrences of X in the code base...
> On July 17, 2015, 8:48 a.m., Alexander Rojas wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 334-335
> > <https://reviews.apache.org/r/36424/diff/2/?file=1010778#file1010778line334>
> >
> > I'm not very sure about doing the `.get()`, since we didn't verify that the objects where properly constructed (in line 320) or checking that they are not `None()`.
good catch!
I'll fix this.
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36424/#review92038
-----------------------------------------------------------
On July 17, 2015, 5:52 a.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36424/
> -----------------------------------------------------------
>
> (Updated July 17, 2015, 5:52 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Cody Maloney.
>
>
> Bugs: MESOS-3035
> https://issues.apache.org/jira/browse/MESOS-3035
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2902
>
> While researching how to execute an arbitrary script
> to detect the Master IP address, it emerged clearly that
> a helper method to execute an arbitrary command/script on
> a node and obtain either stdout or stderr would have been
> useful and avoided a lot of code repetition.
>
> This could not be ultimately used for the purpose at hand,
> but I believe it to be useful enough (particularly, to avoid
> people doing "coding by copy&paste" and/or waste time
> researching the same functionality).
>
> This would also be beneficial in MESOS-2830 and MESOS-2834
> factoring out the remote command execution logic.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp 310cb4f8e4e2faa5545dffd196d7490c868bc5d6
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp f6acb204582a9e696c3b09d4e4c543bb052e97d4
>
> Diff: https://reviews.apache.org/r/36424/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 36424: Created a command executor helper method.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36424/#review92038
-----------------------------------------------------------
3rdparty/libprocess/include/process/subprocess.hpp (lines 302 - 304)
<https://reviews.apache.org/r/36424/#comment145922>
You may ignore this, but I'm not sure if ignoring `cerr` when the command succeds is the way to go. Most logging systems log unto the `clog` which is mostly an alias for the `cerr` file descriptor.
3rdparty/libprocess/include/process/subprocess.hpp (line 307)
<https://reviews.apache.org/r/36424/#comment145919>
I think you should calle it `execute` to follow our concise naming. Moreover, under the context is hard to misunderstand it.
3rdparty/libprocess/include/process/subprocess.hpp (line 314)
<https://reviews.apache.org/r/36424/#comment145929>
ranged based for loops are not yet whitelisted, nor there are any occurrence on the codebase. So I guess it is not yet time to use them.
3rdparty/libprocess/include/process/subprocess.hpp (lines 334 - 335)
<https://reviews.apache.org/r/36424/#comment145927>
I'm not very sure about doing the `.get()`, since we didn't verify that the objects where properly constructed (in line 320) or checking that they are not `None()`.
- Alexander Rojas
On July 17, 2015, 7:52 a.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36424/
> -----------------------------------------------------------
>
> (Updated July 17, 2015, 7:52 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Cody Maloney.
>
>
> Bugs: MESOS-3035
> https://issues.apache.org/jira/browse/MESOS-3035
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2902
>
> While researching how to execute an arbitrary script
> to detect the Master IP address, it emerged clearly that
> a helper method to execute an arbitrary command/script on
> a node and obtain either stdout or stderr would have been
> useful and avoided a lot of code repetition.
>
> This could not be ultimately used for the purpose at hand,
> but I believe it to be useful enough (particularly, to avoid
> people doing "coding by copy&paste" and/or waste time
> researching the same functionality).
>
> This would also be beneficial in MESOS-2830 and MESOS-2834
> factoring out the remote command execution logic.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp 310cb4f8e4e2faa5545dffd196d7490c868bc5d6
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp f6acb204582a9e696c3b09d4e4c543bb052e97d4
>
> Diff: https://reviews.apache.org/r/36424/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 36424: Created a command executor helper method.
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36424/
-----------------------------------------------------------
(Updated July 22, 2015, 6:20 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Cody Maloney, and Paul Brett.
Changes
-------
Addressed Alex comments; improved comments.
Bugs: MESOS-3035
https://issues.apache.org/jira/browse/MESOS-3035
Repository: mesos
Description
-------
Jira: MESOS-2902
While researching how to execute an arbitrary script
to detect the Master IP address, it emerged clearly that
a helper method to execute an arbitrary command/script on
a node and obtain either stdout or stderr would have been
useful and avoided a lot of code repetition.
This could not be ultimately used for the purpose at hand,
but I believe it to be useful enough (particularly, to avoid
people doing "coding by copy&paste" and/or waste time
researching the same functionality).
This would also be beneficial in MESOS-2830 and MESOS-2834
factoring out the remote command execution logic.
Diffs (updated)
-----
3rdparty/libprocess/include/process/subprocess.hpp 310cb4f8e4e2faa5545dffd196d7490c868bc5d6
3rdparty/libprocess/src/tests/subprocess_tests.cpp f6acb204582a9e696c3b09d4e4c543bb052e97d4
Diff: https://reviews.apache.org/r/36424/diff/
Testing
-------
make check
Thanks,
Marco Massenzio
Re: Review Request 36424: Created a command executor helper method.
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36424/
-----------------------------------------------------------
(Updated July 22, 2015, 5:49 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Cody Maloney, and Paul Brett.
Changes
-------
Implemented a timeout; added a couple of tests; implemented comments from benh's review of r/36389
Bugs: MESOS-3035
https://issues.apache.org/jira/browse/MESOS-3035
Repository: mesos
Description
-------
Jira: MESOS-2902
While researching how to execute an arbitrary script
to detect the Master IP address, it emerged clearly that
a helper method to execute an arbitrary command/script on
a node and obtain either stdout or stderr would have been
useful and avoided a lot of code repetition.
This could not be ultimately used for the purpose at hand,
but I believe it to be useful enough (particularly, to avoid
people doing "coding by copy&paste" and/or waste time
researching the same functionality).
This would also be beneficial in MESOS-2830 and MESOS-2834
factoring out the remote command execution logic.
Diffs (updated)
-----
3rdparty/libprocess/include/process/subprocess.hpp 310cb4f8e4e2faa5545dffd196d7490c868bc5d6
3rdparty/libprocess/src/tests/subprocess_tests.cpp f6acb204582a9e696c3b09d4e4c543bb052e97d4
Diff: https://reviews.apache.org/r/36424/diff/
Testing
-------
make check
Thanks,
Marco Massenzio
Re: Review Request 36424: Created a command executor helper method.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36424/#review92232
-----------------------------------------------------------
3rdparty/libprocess/include/process/subprocess.hpp (line 364)
<https://reviews.apache.org/r/36424/#comment146283>
Isn't `++cmdArgs.begin()` the same `cmdArgs.end()` ? I feel that pre-increment looks quite weird.
If you still prefer the given style, I think `cmdArgs.begin() + 1` is still more readable and doesn't add complexity.
- Alexander Rojas
On July 18, 2015, 4 a.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36424/
> -----------------------------------------------------------
>
> (Updated July 18, 2015, 4 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Cody Maloney, and Paul Brett.
>
>
> Bugs: MESOS-3035
> https://issues.apache.org/jira/browse/MESOS-3035
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2902
>
> While researching how to execute an arbitrary script
> to detect the Master IP address, it emerged clearly that
> a helper method to execute an arbitrary command/script on
> a node and obtain either stdout or stderr would have been
> useful and avoided a lot of code repetition.
>
> This could not be ultimately used for the purpose at hand,
> but I believe it to be useful enough (particularly, to avoid
> people doing "coding by copy&paste" and/or waste time
> researching the same functionality).
>
> This would also be beneficial in MESOS-2830 and MESOS-2834
> factoring out the remote command execution logic.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp 310cb4f8e4e2faa5545dffd196d7490c868bc5d6
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp f6acb204582a9e696c3b09d4e4c543bb052e97d4
>
> Diff: https://reviews.apache.org/r/36424/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 36424: Created a command executor helper method.
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36424/
-----------------------------------------------------------
(Updated July 18, 2015, 2 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Cody Maloney, and Paul Brett.
Changes
-------
Forgotten to convert comments to javadoc format.
Bugs: MESOS-3035
https://issues.apache.org/jira/browse/MESOS-3035
Repository: mesos
Description
-------
Jira: MESOS-2902
While researching how to execute an arbitrary script
to detect the Master IP address, it emerged clearly that
a helper method to execute an arbitrary command/script on
a node and obtain either stdout or stderr would have been
useful and avoided a lot of code repetition.
This could not be ultimately used for the purpose at hand,
but I believe it to be useful enough (particularly, to avoid
people doing "coding by copy&paste" and/or waste time
researching the same functionality).
This would also be beneficial in MESOS-2830 and MESOS-2834
factoring out the remote command execution logic.
Diffs (updated)
-----
3rdparty/libprocess/include/process/subprocess.hpp 310cb4f8e4e2faa5545dffd196d7490c868bc5d6
3rdparty/libprocess/src/tests/subprocess_tests.cpp f6acb204582a9e696c3b09d4e4c543bb052e97d4
Diff: https://reviews.apache.org/r/36424/diff/
Testing
-------
make check
Thanks,
Marco Massenzio
Re: Review Request 36424: Created a command executor helper method.
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36424/
-----------------------------------------------------------
(Updated July 18, 2015, 1:55 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Cody Maloney, and Paul Brett.
Changes
-------
First pass, after initial comments, refactoring to emit a CommandResult.
Bugs: MESOS-3035
https://issues.apache.org/jira/browse/MESOS-3035
Repository: mesos
Description
-------
Jira: MESOS-2902
While researching how to execute an arbitrary script
to detect the Master IP address, it emerged clearly that
a helper method to execute an arbitrary command/script on
a node and obtain either stdout or stderr would have been
useful and avoided a lot of code repetition.
This could not be ultimately used for the purpose at hand,
but I believe it to be useful enough (particularly, to avoid
people doing "coding by copy&paste" and/or waste time
researching the same functionality).
This would also be beneficial in MESOS-2830 and MESOS-2834
factoring out the remote command execution logic.
Diffs (updated)
-----
3rdparty/libprocess/include/process/subprocess.hpp 310cb4f8e4e2faa5545dffd196d7490c868bc5d6
3rdparty/libprocess/src/tests/subprocess_tests.cpp f6acb204582a9e696c3b09d4e4c543bb052e97d4
Diff: https://reviews.apache.org/r/36424/diff/
Testing
-------
make check
Thanks,
Marco Massenzio
Re: Review Request 36424: Created a command executor helper method.
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36424/
-----------------------------------------------------------
(Updated July 17, 2015, 5:52 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Cody Maloney.
Changes
-------
Added comments from Ben M.
Bugs: MESOS-3035
https://issues.apache.org/jira/browse/MESOS-3035
Repository: mesos
Description (updated)
-------
Jira: MESOS-2902
While researching how to execute an arbitrary script
to detect the Master IP address, it emerged clearly that
a helper method to execute an arbitrary command/script on
a node and obtain either stdout or stderr would have been
useful and avoided a lot of code repetition.
This could not be ultimately used for the purpose at hand,
but I believe it to be useful enough (particularly, to avoid
people doing "coding by copy&paste" and/or waste time
researching the same functionality).
This would also be beneficial in MESOS-2830 and MESOS-2834
factoring out the remote command execution logic.
Diffs
-----
3rdparty/libprocess/include/process/subprocess.hpp 310cb4f8e4e2faa5545dffd196d7490c868bc5d6
3rdparty/libprocess/src/tests/subprocess_tests.cpp f6acb204582a9e696c3b09d4e4c543bb052e97d4
Diff: https://reviews.apache.org/r/36424/diff/
Testing
-------
make check
Thanks,
Marco Massenzio
Re: Review Request 36424: Created a command executor helper method.
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36424/#review92028
-----------------------------------------------------------
>From an email thread [here](http://www.mail-archive.com/dev@mesos.apache.org/msg32844.html):
> (1) Discard semantics: with subprocess the caller has the ability to kill
the process through s.pid. However, with 'execute' that you've introduced,
if the caller discards the future, we should be killing the process tree
we've forked to clean up.
Nice catch - I may need some guidance on how to do that, but I'll give it a
shot.
> (2) As a general note, `Future<Try<string>>` as a type tends to be a bit
confusing for folks as one has to resort to reading comments to figure out
the difference between f.isFailed() and f.get().isError(). When we talk
about "readability" we're generally including things like this, where it
isn't obvious to the "reader" of the code. Similar to paul's approach, I'd
suggest having a small struct to capture the data of a completed command
(i.e. status, out, err) to make this very clear. e.g. `Future<Output>`
Make sense?
Yes!
I was actually thinking about even creating a Protobuf for this (some kind
of CommandResultInfo) and use that.
> (3) Subprocess has two overloads, path+args (exec-style), and command (sh
> -c "command" style). Your documentation says "sh -c" but the code is just
> directly passing through to path+args subprocess, so it's not going through
> sh -c at all. Something I'm missing?
Bad comment (I'd originally started down the 'sh -c' path, then figured out
it was not strictly necessary).
I'll fix that.
> Lastly, it would be great to see the caller code related to this, spot
checking I see a number of places that are unnecessarily tedious (e.g.
perf.cpp):
```
// Start reading from stdout and stderr now. We don't use stderr
// but must read from it to avoid the subprocess blocking on the
// pipe.
output.push_back(io::read(perf.get().out().get()));
output.push_back(io::read(perf.get().err().get()));
// Wait for the process to exit.
perf.get().status()
.onAny(defer(self(), &Self::_sample, lambda::_1));
```
> vs
```
process::await(
perf.get().status(),
io::read(perf.get().out().get()),
io::read(perf.get().err().get()))
.then(defer(self(), &Self::_sample, lambda::_1));
```
> There is no need to wait for the command first before continuing, can just
wait for everything. Having something like what you're proposing seems even
cleaner, but it would be helpful to first start with the above improvement
to get a better sense of all the use cases, and whether just adding a
`.join()` that returns `Future<Output>` gets us all the way there. I propose
this first because the structural simplifications needed will be very
similar to those from `process::executeCommand`.
Uhm - this is assuming a deeper understanding of libprocess than I
currently have: let me ponder this a bit and I may reach out for more
advice.
And, yes, my code was largely based on `perf.cpp` blueprint.
- Marco Massenzio
On July 14, 2015, 4:21 a.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36424/
> -----------------------------------------------------------
>
> (Updated July 14, 2015, 4:21 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Cody Maloney.
>
>
> Bugs: MESOS-3035
> https://issues.apache.org/jira/browse/MESOS-3035
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2902
>
> While researching how to execute an arbitrary script
> to detect the Master IP address, it emerged clearly that
> a helper method to execute an arbitrary command/script on
> a node and obtain either stdout or stderr would have been
> useful and avoided a lot of code repetition.
>
> This could not be ultimately used for the purpose at hand,
> but I believe it to be useful enough (particularly, to avoid
> people doing "coding by copy&paste" and/or waste time
> researching the same functionality).
>
> This would also be beneficial in MESOS-2830, factoring out the remote command execution logic.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/subprocess.hpp 310cb4f8e4e2faa5545dffd196d7490c868bc5d6
> 3rdparty/libprocess/src/tests/subprocess_tests.cpp f6acb204582a9e696c3b09d4e4c543bb052e97d4
>
> Diff: https://reviews.apache.org/r/36424/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Marco Massenzio
>
>