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

Reply via email to