You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Anand Mazumdar <ma...@gmail.com> on 2016/04/09 01:29:47 UTC
Re: Review Request 44427: Added --http_command_executor flag.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44427/#review127910
-----------------------------------------------------------
Looks pretty good. Mostly minor comments around indent.
docs/configuration.md (lines 1251 - 1254)
<https://reviews.apache.org/r/44427/#comment191296>
How about:
```
<td>
The underlying executor library to be used for the command executor. If set to <code>true</code>, the command executor would use the HTTP based executor library to interact with the Mesos agent. If set to <code>false</code>, the driver based implementation would be used.
<b>NOTE</b>: This flag is *experimental* and should not be used in production yet. (default:false)
</td>
```
src/slave/flags.cpp (lines 696 - 698)
<https://reviews.apache.org/r/44427/#comment191298>
This would also need to be updated as per my earlier comment.
src/slave/slave.cpp (line 3691)
<https://reviews.apache.org/r/44427/#comment191283>
Two space indent here.
src/slave/slave.cpp (line 3694)
<https://reviews.apache.org/r/44427/#comment191284>
2 space indent here.
src/slave/slave.cpp (line 5640)
<https://reviews.apache.org/r/44427/#comment191285>
We typically don't spill partial arguments from a function invocation.
How about:
```cpp
executorPath = os::realpath(
path::join(slave->flags.launcher_dir, ..));
```
src/slave/slave.cpp (line 5643)
<https://reviews.apache.org/r/44427/#comment191289>
Same as above.
- Anand Mazumdar
On March 5, 2016, 4:37 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44427/
> -----------------------------------------------------------
>
> (Updated March 5, 2016, 4:37 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Vinod Kone.
>
>
> Bugs: MESOS-3558
> https://issues.apache.org/jira/browse/MESOS-3558
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added --http_command_executor flag.
>
>
> Diffs
> -----
>
> docs/configuration.md 305ba2c801c2060db6dcb4ef83c1043aaa7d520c
> src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388
> src/slave/flags.cpp 6e3fd69c06eefd40bc0e5c222ea72f34144c5534
> src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697
>
> Diff: https://reviews.apache.org/r/44427/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 44427: Added --http_command_executor flag.
Posted by Anand Mazumdar <ma...@gmail.com>.
> On April 8, 2016, 11:29 p.m., Anand Mazumdar wrote:
> > src/slave/slave.cpp, line 5640
> > <https://reviews.apache.org/r/44427/diff/1/?file=1281966#file1281966line5640>
> >
> > We typically don't spill partial arguments from a function invocation.
> >
> > How about:
> >
> > ```cpp
> > executorPath = os::realpath(
> > path::join(slave->flags.launcher_dir, ..));
> > ```
>
> Qian Zhang wrote:
> Agree, but I think it should 4 space indent, see the followings as example:
> https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L5901:L5902
> https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L5955:L5956
Yep, was a bad copy paste typo on my part. Thanks for noticing it and updating the patch with the correct fix!
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44427/#review127910
-----------------------------------------------------------
On April 9, 2016, 8:47 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44427/
> -----------------------------------------------------------
>
> (Updated April 9, 2016, 8:47 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Vinod Kone.
>
>
> Bugs: MESOS-3558
> https://issues.apache.org/jira/browse/MESOS-3558
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added --http_command_executor flag.
>
>
> Diffs
> -----
>
> docs/configuration.md ba00ec563c449345effb3114111812601addcfc2
> src/slave/flags.hpp 300db49100d989d6c0409766b1341cb956ea1631
> src/slave/flags.cpp dd7bc9a48dfd8481336a2d2ec0beecd19a342644
> src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3
>
> Diff: https://reviews.apache.org/r/44427/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 44427: Added --http_command_executor flag.
Posted by Qian Zhang <zh...@cn.ibm.com>.
> On April 9, 2016, 7:29 a.m., Anand Mazumdar wrote:
> > src/slave/slave.cpp, line 5640
> > <https://reviews.apache.org/r/44427/diff/1/?file=1281966#file1281966line5640>
> >
> > We typically don't spill partial arguments from a function invocation.
> >
> > How about:
> >
> > ```cpp
> > executorPath = os::realpath(
> > path::join(slave->flags.launcher_dir, ..));
> > ```
Agree, but I think it should 4 space indent, see the followings as example:
https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L5901:L5902
https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L5955:L5956
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44427/#review127910
-----------------------------------------------------------
On March 6, 2016, 12:37 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44427/
> -----------------------------------------------------------
>
> (Updated March 6, 2016, 12:37 a.m.)
>
>
> Review request for mesos, Anand Mazumdar and Vinod Kone.
>
>
> Bugs: MESOS-3558
> https://issues.apache.org/jira/browse/MESOS-3558
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added --http_command_executor flag.
>
>
> Diffs
> -----
>
> docs/configuration.md 305ba2c801c2060db6dcb4ef83c1043aaa7d520c
> src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388
> src/slave/flags.cpp 6e3fd69c06eefd40bc0e5c222ea72f34144c5534
> src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697
>
> Diff: https://reviews.apache.org/r/44427/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>