-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36389/#review91723
-----------------------------------------------------------
src/slave/flags.hpp (line 117)
<https://reviews.apache.org/r/36389/#comment145304>
Just for my own education purpose :-), Is there a reason for a newline here?
src/slave/slave.cpp (line 151)
<https://reviews.apache.org/r/36389/#comment145297>
Do you think it makes sense to capture the default value in a single location? Otherwise
it's hardcoded in two places (here and in flags.cpp)
src/slave/slave.cpp (lines 4691 - 4694)
<https://reviews.apache.org/r/36389/#comment145310>
wouldn't const std::vector<string> args(command.arguments()); work here?
src/slave/slave.cpp (line 4698)
<https://reviews.apache.org/r/36389/#comment145298>
If you make this review dependent on /r/36424 then RB should be smart enough to apply
the patch from there before apllying and building this one.
That should eliminate the need of having a temporary code in the review.
src/slave/slave.cpp (line 4702)
<https://reviews.apache.org/r/36389/#comment145311>
if we are using command.value() here we might as well use command.args() and ditch the
args variable altogether.
src/slave/slave.cpp (line 4743)
<https://reviews.apache.org/r/36389/#comment145302>
Is it OK to return HTTP 200 if the command returned a non-zero error code?
In the statements above, for exaple, the error code is returned if the command execution
failed (albeit, the reasons for a failure are different).
This approach makes the HTTP 200 insufficient for verifying successful execution of the
command.
src/slave/slave.cpp (line 4755)
<https://reviews.apache.org/r/36389/#comment145299>
Shouldn't there be an equivalent of an assert here if we never expect this to happen?
Something like this:
if (response.isReady()) {
ASSERT
}
return http::BadRequest ...
Unless, there is possibility of a race where the result becomes ready right at the 15th
second (in a separate thread) and by the time this lambda is executed the response becomes
actually (and legitimately) ready.
- Artem Harutyunyan
On July 14, 2015, 4:20 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36389/
> -----------------------------------------------------------
>
> (Updated July 14, 2015, 4:20 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Cody Maloney.
>
>
> Bugs: MESOS-2830
> https://issues.apache.org/jira/browse/MESOS-2830
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2830
>
> Under certain (maintenance) circumnstance, it may be necessary
> (or desirable) to execute arbitrary operator's commands on the
> slave (the entire fleet or a subset thereof) bypassing the Mesos
> Task execution mechanism; this may typically be necessary for
> maintenance and/or emergency actions.
>
> This patch adds an HTTP endpoint (/execute) which accepts a
> JSON-encoded CommandInfo structure and executes the given
> command (with optional arguments).
>
> The output of the command (along with potentially any stderr
> messages) is returned JSON-encoded in the Response.
>
> For more details, see the design doc at:
> https://goo.gl/4npTMU
>
>
> Diffs
> -----
>
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/main.cpp 8008430e98773d8be9ba5ac6385cffb2e351932a
> src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a
> src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4
>
> Diff: https://reviews.apache.org/r/36389/diff/
>
>
> Testing
> -------
>
> make check
>
> lots of manual testing (using Postman, REST client)
>
>
> Thanks,
>
> Marco Massenzio
>
>
|