mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Timothy Chen" <tnac...@apache.org>
Subject Re: Review Request 26622: Command Executor is broken when used with shell=false
Date Thu, 23 Oct 2014 16:03:44 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26622/#review57997
-----------------------------------------------------------



src/slave/slave.cpp
<https://reviews.apache.org/r/26622/#comment98911>

    Thanks for the fix! I wonder if we should just fix the command executor instead?
    
    I usually don't like mingling with user API input as it always confuses whoever using
this, and leads to people start asking mailing lists and reading code because their arguemnts
are changed.
    
    Shell=false was added for a reason and I think we shouldn't just change it for them, did
you see why we can't just support this variation in the command executor?


- Timothy Chen


On Oct. 18, 2014, 4:40 a.m., R.B. Boyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26622/
> -----------------------------------------------------------
> 
> (Updated Oct. 18, 2014, 4:40 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-1873
>     https://issues.apache.org/jira/browse/MESOS-1873
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Basically if you use "shell=false" with a non-empty argument list and the Command Executor
it is completely broken.
> 
> When we clone the env vars to fork "mesos-executor" all of the original cmd args are
cloned as well (unintentionally) due to some protocol-buffer merge shenanigans.
> 
> Don't pass task-related arguments to mesos-executor.
> 
> The description on the linked jira ticket goes into more detail.
> 
> 
> Diffs
> -----
> 
>   src/slave/slave.cpp 0e342ed 
>   src/tests/slave_tests.cpp f585bdd 
> 
> Diff: https://reviews.apache.org/r/26622/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> added new test "SlaveTest.GetExecutorInfo" verifying the explicit desired behavior.
> 
> 
> Thanks,
> 
> R.B. Boyer
> 
>


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