mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marco Massenzio" <ma...@mesosphere.io>
Subject Re: Review Request 36424: Created a command executor helper method.
Date Fri, 17 Jul 2015 20:51:43 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message