You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Qian Zhang <zh...@cn.ibm.com> on 2016/02/18 02:09:23 UTC

Review Request 43701: Added a command executor based on the new V1 API.

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

Review request for mesos, Anand Mazumdar and Vinod Kone.


Bugs: MESOS-3558
    https://issues.apache.org/jira/browse/MESOS-3558


Repository: mesos


Description
-------

Added a command executor based on the new V1 API.


Diffs
-----

  docs/configuration.md 3d8236822af688a88a9f9f357c67c03d7d60fdd9 
  include/mesos/v1/mesos.proto d909e60ddfd8e3ba2075f82c372edde04cd99d54 
  src/Makefile.am 54ebe91643a17017c00cdbd5dfc8ce1a021579d5 
  src/launcher/http_executor.cpp PRE-CREATION 
  src/slave/flags.hpp bd52b4f9816b271a9d2728ae6915f8e24b74d716 
  src/slave/flags.cpp d4b4e5201bb3a7e8edd0ab2481328fc91cd8f974 
  src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 

Diff: https://reviews.apache.org/r/43701/diff/


Testing
-------

make check


Thanks,

Qian Zhang

Re: Review Request 43701: Added a command executor based on the new V1 API.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43701/#review120046
-----------------------------------------------------------



Patch looks great!

Reviews applied: [43701]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 20, 2016, 1:44 p.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43701/
> -----------------------------------------------------------
> 
> (Updated Feb. 20, 2016, 1:44 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 a command executor based on the new V1 API.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md b04e873009dc783cc50eb02a459f0587d020ad23 
>   include/mesos/v1/mesos.proto e4224afe2245d649afa1a6c97bae26c215e6fada 
>   src/Makefile.am 27aec37524aa33211e0ca4594e127ebb4279e9b0 
>   src/launcher/http_executor.cpp PRE-CREATION 
>   src/slave/flags.hpp 54c1a69d8777f417cdd8f73ce638447d9951ab61 
>   src/slave/flags.cpp 855812e9f7cb4b96d4297f4bd5ac5de7f1d3c39a 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
> 
> Diff: https://reviews.apache.org/r/43701/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43701: Added a command executor based on the new V1 API.

Posted by Qian Zhang <zh...@cn.ibm.com>.

> On Feb. 29, 2016, 10:24 a.m., Vinod Kone wrote:
> > Thanks for working on this Qian!
> > 
> > It's really hard to tell what changes were made to the http command executor that are different from the command executor. I would suggest you to split this into multiple reviews to make reviewers' life easy.
> > 
> > 1) Add http command executor to make files. Just copy executor.cpp to http_command_executor.cpp without any changes.
> > 2) Update http_command_executor.cpp to use v1 API.
> > 3) Make changes to flags.cpp and slave.cpp.
> > 4) Update/parameterize tests (slave recovery tests?) to use http command executor.

Sure Vinod, I will discard this patch, and upload multiple sub-patches accordingly.


- Qian


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


On Feb. 20, 2016, 9:44 p.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43701/
> -----------------------------------------------------------
> 
> (Updated Feb. 20, 2016, 9:44 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 a command executor based on the new V1 API.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md b04e873009dc783cc50eb02a459f0587d020ad23 
>   include/mesos/v1/mesos.proto e4224afe2245d649afa1a6c97bae26c215e6fada 
>   src/Makefile.am 27aec37524aa33211e0ca4594e127ebb4279e9b0 
>   src/launcher/http_executor.cpp PRE-CREATION 
>   src/slave/flags.hpp 54c1a69d8777f417cdd8f73ce638447d9951ab61 
>   src/slave/flags.cpp 855812e9f7cb4b96d4297f4bd5ac5de7f1d3c39a 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
> 
> Diff: https://reviews.apache.org/r/43701/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43701: Added a command executor based on the new V1 API.

Posted by Qian Zhang <zh...@cn.ibm.com>.

> On Feb. 29, 2016, 10:24 a.m., Vinod Kone wrote:
> > Thanks for working on this Qian!
> > 
> > It's really hard to tell what changes were made to the http command executor that are different from the command executor. I would suggest you to split this into multiple reviews to make reviewers' life easy.
> > 
> > 1) Add http command executor to make files. Just copy executor.cpp to http_command_executor.cpp without any changes.
> > 2) Update http_command_executor.cpp to use v1 API.
> > 3) Make changes to flags.cpp and slave.cpp.
> > 4) Update/parameterize tests (slave recovery tests?) to use http command executor.
> 
> Qian Zhang wrote:
>     Sure Vinod, I will discard this patch, and upload multiple sub-patches accordingly.
> 
> Vinod Kone wrote:
>     Hey any updates on this?

Sorry for the late, I have uploaded patches for 1), 2) and 3), and I am working on 4), can you please help review the first 3 patches? Thanks. 
https://reviews.apache.org/r/44423/


- Qian


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


On Feb. 20, 2016, 9:44 p.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43701/
> -----------------------------------------------------------
> 
> (Updated Feb. 20, 2016, 9:44 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 a command executor based on the new V1 API.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md b04e873009dc783cc50eb02a459f0587d020ad23 
>   include/mesos/v1/mesos.proto e4224afe2245d649afa1a6c97bae26c215e6fada 
>   src/Makefile.am 27aec37524aa33211e0ca4594e127ebb4279e9b0 
>   src/launcher/http_executor.cpp PRE-CREATION 
>   src/slave/flags.hpp 54c1a69d8777f417cdd8f73ce638447d9951ab61 
>   src/slave/flags.cpp 855812e9f7cb4b96d4297f4bd5ac5de7f1d3c39a 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
> 
> Diff: https://reviews.apache.org/r/43701/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43701: Added a command executor based on the new V1 API.

Posted by Vinod Kone <vi...@gmail.com>.

> On Feb. 29, 2016, 2:24 a.m., Vinod Kone wrote:
> > Thanks for working on this Qian!
> > 
> > It's really hard to tell what changes were made to the http command executor that are different from the command executor. I would suggest you to split this into multiple reviews to make reviewers' life easy.
> > 
> > 1) Add http command executor to make files. Just copy executor.cpp to http_command_executor.cpp without any changes.
> > 2) Update http_command_executor.cpp to use v1 API.
> > 3) Make changes to flags.cpp and slave.cpp.
> > 4) Update/parameterize tests (slave recovery tests?) to use http command executor.
> 
> Qian Zhang wrote:
>     Sure Vinod, I will discard this patch, and upload multiple sub-patches accordingly.

Hey any updates on this?


- Vinod


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


On Feb. 20, 2016, 1:44 p.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43701/
> -----------------------------------------------------------
> 
> (Updated Feb. 20, 2016, 1:44 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 a command executor based on the new V1 API.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md b04e873009dc783cc50eb02a459f0587d020ad23 
>   include/mesos/v1/mesos.proto e4224afe2245d649afa1a6c97bae26c215e6fada 
>   src/Makefile.am 27aec37524aa33211e0ca4594e127ebb4279e9b0 
>   src/launcher/http_executor.cpp PRE-CREATION 
>   src/slave/flags.hpp 54c1a69d8777f417cdd8f73ce638447d9951ab61 
>   src/slave/flags.cpp 855812e9f7cb4b96d4297f4bd5ac5de7f1d3c39a 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
> 
> Diff: https://reviews.apache.org/r/43701/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43701: Added a command executor based on the new V1 API.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43701/#review121177
-----------------------------------------------------------



Thanks for working on this Qian!

It's really hard to tell what changes were made to the http command executor that are different from the command executor. I would suggest you to split this into multiple reviews to make reviewers' life easy.

1) Add http command executor to make files. Just copy executor.cpp to http_command_executor.cpp without any changes.
2) Update http_command_executor.cpp to use v1 API.
3) Make changes to flags.cpp and slave.cpp.
4) Update/parameterize tests (slave recovery tests?) to use http command executor.

- Vinod Kone


On Feb. 20, 2016, 1:44 p.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43701/
> -----------------------------------------------------------
> 
> (Updated Feb. 20, 2016, 1:44 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 a command executor based on the new V1 API.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md b04e873009dc783cc50eb02a459f0587d020ad23 
>   include/mesos/v1/mesos.proto e4224afe2245d649afa1a6c97bae26c215e6fada 
>   src/Makefile.am 27aec37524aa33211e0ca4594e127ebb4279e9b0 
>   src/launcher/http_executor.cpp PRE-CREATION 
>   src/slave/flags.hpp 54c1a69d8777f417cdd8f73ce638447d9951ab61 
>   src/slave/flags.cpp 855812e9f7cb4b96d4297f4bd5ac5de7f1d3c39a 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
> 
> Diff: https://reviews.apache.org/r/43701/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43701: Added a command executor based on the new V1 API.

Posted by Qian Zhang <zh...@cn.ibm.com>.

> On Feb. 29, 2016, 1:42 a.m., Shuai Lin wrote:
> > src/launcher/http_executor.cpp, line 82
> > <https://reviews.apache.org/r/43701/diff/2/?file=1263570#file1263570line82>
> >
> >     Maybe we should also add some tests that launches tasks with this new http cmd executor?

Yes, agree!


- Qian


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


On Feb. 20, 2016, 9:44 p.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43701/
> -----------------------------------------------------------
> 
> (Updated Feb. 20, 2016, 9:44 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 a command executor based on the new V1 API.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md b04e873009dc783cc50eb02a459f0587d020ad23 
>   include/mesos/v1/mesos.proto e4224afe2245d649afa1a6c97bae26c215e6fada 
>   src/Makefile.am 27aec37524aa33211e0ca4594e127ebb4279e9b0 
>   src/launcher/http_executor.cpp PRE-CREATION 
>   src/slave/flags.hpp 54c1a69d8777f417cdd8f73ce638447d9951ab61 
>   src/slave/flags.cpp 855812e9f7cb4b96d4297f4bd5ac5de7f1d3c39a 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
> 
> Diff: https://reviews.apache.org/r/43701/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43701: Added a command executor based on the new V1 API.

Posted by Shuai Lin <li...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43701/#review121155
-----------------------------------------------------------




src/launcher/http_executor.cpp (line 82)
<https://reviews.apache.org/r/43701/#comment182810>

    Maybe we should also add some tests that launches tasks with this new http cmd executor?


- Shuai Lin


On Feb. 20, 2016, 1:44 p.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43701/
> -----------------------------------------------------------
> 
> (Updated Feb. 20, 2016, 1:44 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 a command executor based on the new V1 API.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md b04e873009dc783cc50eb02a459f0587d020ad23 
>   include/mesos/v1/mesos.proto e4224afe2245d649afa1a6c97bae26c215e6fada 
>   src/Makefile.am 27aec37524aa33211e0ca4594e127ebb4279e9b0 
>   src/launcher/http_executor.cpp PRE-CREATION 
>   src/slave/flags.hpp 54c1a69d8777f417cdd8f73ce638447d9951ab61 
>   src/slave/flags.cpp 855812e9f7cb4b96d4297f4bd5ac5de7f1d3c39a 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
> 
> Diff: https://reviews.apache.org/r/43701/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43701: Added a command executor based on the new V1 API.

Posted by Qian Zhang <zh...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43701/
-----------------------------------------------------------

(Updated Feb. 20, 2016, 9:44 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 a command executor based on the new V1 API.


Diffs (updated)
-----

  docs/configuration.md b04e873009dc783cc50eb02a459f0587d020ad23 
  include/mesos/v1/mesos.proto e4224afe2245d649afa1a6c97bae26c215e6fada 
  src/Makefile.am 27aec37524aa33211e0ca4594e127ebb4279e9b0 
  src/launcher/http_executor.cpp PRE-CREATION 
  src/slave/flags.hpp 54c1a69d8777f417cdd8f73ce638447d9951ab61 
  src/slave/flags.cpp 855812e9f7cb4b96d4297f4bd5ac5de7f1d3c39a 
  src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 

Diff: https://reviews.apache.org/r/43701/diff/


Testing
-------

make check


Thanks,

Qian Zhang

Re: Review Request 43701: Added a command executor based on the new V1 API.

Posted by Shuai Lin <li...@gmail.com>.

> On Feb. 18, 2016, 4:07 p.m., Shuai Lin wrote:
> > src/slave/slave.cpp, line 3709
> > <https://reviews.apache.org/r/43701/diff/1/?file=1253453#file1253453line3709>
> >
> >     Instead of repeating the `if (flags.http_command_executor)...` logic multiple times, I would prefer use a temp variable to store either `mesos-executor` or `mesos-http-executor`.
> 
> Qian Zhang wrote:
>     I am not sure if I get your point correctly, can you please clarify how to use the temp var? BTW, such logic is just repeated twice in `Slave::getExecutorInfo`, it seems not a big deal to me :-)

Fair enough.


- Shuai


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


On Feb. 20, 2016, 1:44 p.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43701/
> -----------------------------------------------------------
> 
> (Updated Feb. 20, 2016, 1:44 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 a command executor based on the new V1 API.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md b04e873009dc783cc50eb02a459f0587d020ad23 
>   include/mesos/v1/mesos.proto e4224afe2245d649afa1a6c97bae26c215e6fada 
>   src/Makefile.am 27aec37524aa33211e0ca4594e127ebb4279e9b0 
>   src/launcher/http_executor.cpp PRE-CREATION 
>   src/slave/flags.hpp 54c1a69d8777f417cdd8f73ce638447d9951ab61 
>   src/slave/flags.cpp 855812e9f7cb4b96d4297f4bd5ac5de7f1d3c39a 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
> 
> Diff: https://reviews.apache.org/r/43701/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43701: Added a command executor based on the new V1 API.

Posted by Qian Zhang <zh...@cn.ibm.com>.

> On Feb. 19, 2016, 12:07 a.m., Shuai Lin wrote:
> > src/slave/flags.cpp, line 693
> > <https://reviews.apache.org/r/43701/diff/1/?file=1253452#file1253452line693>
> >
> >     One space before `\n`, otherwise the word would be mixed with the first word of the next line.

I do not think we will have such issue, you can take a look at the all other existing flags, they all do the same way. And here is the output when I run "./src/mesos-slave --help":
# ./src/mesos-slave --help
Usage: lt-mesos-slave [options]
...
--[no-]http_command_executor                      Enable mesos containerizer to use HTTP command executor which uses
                                                  executor HTTP API to interact with Mesos agent. If false, the old
                                                  command executor which uses executor driver API will be used.
                                                  default: false)
...

I think the output is desired.


> On Feb. 19, 2016, 12:07 a.m., Shuai Lin wrote:
> > src/slave/slave.cpp, line 3709
> > <https://reviews.apache.org/r/43701/diff/1/?file=1253453#file1253453line3709>
> >
> >     Instead of repeating the `if (flags.http_command_executor)...` logic multiple times, I would prefer use a temp variable to store either `mesos-executor` or `mesos-http-executor`.

I am not sure if I get your point correctly, can you please clarify how to use the temp var? BTW, such logic is just repeated twice in `Slave::getExecutorInfo`, it seems not a big deal to me :-)


- Qian


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


On Feb. 18, 2016, 9:09 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43701/
> -----------------------------------------------------------
> 
> (Updated Feb. 18, 2016, 9:09 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 a command executor based on the new V1 API.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 3d8236822af688a88a9f9f357c67c03d7d60fdd9 
>   include/mesos/v1/mesos.proto d909e60ddfd8e3ba2075f82c372edde04cd99d54 
>   src/Makefile.am 54ebe91643a17017c00cdbd5dfc8ce1a021579d5 
>   src/launcher/http_executor.cpp PRE-CREATION 
>   src/slave/flags.hpp bd52b4f9816b271a9d2728ae6915f8e24b74d716 
>   src/slave/flags.cpp d4b4e5201bb3a7e8edd0ab2481328fc91cd8f974 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
> 
> Diff: https://reviews.apache.org/r/43701/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43701: Added a command executor based on the new V1 API.

Posted by Shuai Lin <li...@gmail.com>.

> On Feb. 18, 2016, 4:07 p.m., Shuai Lin wrote:
> > src/slave/flags.cpp, line 693
> > <https://reviews.apache.org/r/43701/diff/1/?file=1253452#file1253452line693>
> >
> >     One space before `\n`, otherwise the word would be mixed with the first word of the next line.
> 
> Qian Zhang wrote:
>     I do not think we will have such issue, you can take a look at the all other existing flags, they all do the same way. And here is the output when I run "./src/mesos-slave --help":
>     # ./src/mesos-slave --help
>     Usage: lt-mesos-slave [options]
>     ...
>     --[no-]http_command_executor                      Enable mesos containerizer to use HTTP command executor which uses
>                                                       executor HTTP API to interact with Mesos agent. If false, the old
>                                                       command executor which uses executor driver API will be used.
>                                                       default: false)
>     ...
>     
>     I think the output is desired.

You're right. I didn't note there is an explicit `"\n"` in the end of every line.


- Shuai


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


On Feb. 20, 2016, 1:44 p.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43701/
> -----------------------------------------------------------
> 
> (Updated Feb. 20, 2016, 1:44 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 a command executor based on the new V1 API.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md b04e873009dc783cc50eb02a459f0587d020ad23 
>   include/mesos/v1/mesos.proto e4224afe2245d649afa1a6c97bae26c215e6fada 
>   src/Makefile.am 27aec37524aa33211e0ca4594e127ebb4279e9b0 
>   src/launcher/http_executor.cpp PRE-CREATION 
>   src/slave/flags.hpp 54c1a69d8777f417cdd8f73ce638447d9951ab61 
>   src/slave/flags.cpp 855812e9f7cb4b96d4297f4bd5ac5de7f1d3c39a 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
> 
> Diff: https://reviews.apache.org/r/43701/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43701: Added a command executor based on the new V1 API.

Posted by Shuai Lin <li...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43701/#review119623
-----------------------------------------------------------




src/slave/flags.cpp (line 693)
<https://reviews.apache.org/r/43701/#comment180973>

    One space before `\n`, otherwise the word would be mixed with the first word of the next line.



src/slave/flags.cpp (line 694)
<https://reviews.apache.org/r/43701/#comment180974>

    ditto



src/slave/slave.cpp (line 3709)
<https://reviews.apache.org/r/43701/#comment180972>

    Instead of repeating the `if (flags.http_command_executor)...` logic multiple times, I would prefer use a temp variable to store either `mesos-executor` or `mesos-http-executor`.


- Shuai Lin


On Feb. 18, 2016, 1:09 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43701/
> -----------------------------------------------------------
> 
> (Updated Feb. 18, 2016, 1:09 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 a command executor based on the new V1 API.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 3d8236822af688a88a9f9f357c67c03d7d60fdd9 
>   include/mesos/v1/mesos.proto d909e60ddfd8e3ba2075f82c372edde04cd99d54 
>   src/Makefile.am 54ebe91643a17017c00cdbd5dfc8ce1a021579d5 
>   src/launcher/http_executor.cpp PRE-CREATION 
>   src/slave/flags.hpp bd52b4f9816b271a9d2728ae6915f8e24b74d716 
>   src/slave/flags.cpp d4b4e5201bb3a7e8edd0ab2481328fc91cd8f974 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
> 
> Diff: https://reviews.apache.org/r/43701/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43701: Added a command executor based on the new V1 API.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43701/#review119583
-----------------------------------------------------------



Patch looks great!

Reviews applied: [43701]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 18, 2016, 1:09 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43701/
> -----------------------------------------------------------
> 
> (Updated Feb. 18, 2016, 1:09 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 a command executor based on the new V1 API.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 3d8236822af688a88a9f9f357c67c03d7d60fdd9 
>   include/mesos/v1/mesos.proto d909e60ddfd8e3ba2075f82c372edde04cd99d54 
>   src/Makefile.am 54ebe91643a17017c00cdbd5dfc8ce1a021579d5 
>   src/launcher/http_executor.cpp PRE-CREATION 
>   src/slave/flags.hpp bd52b4f9816b271a9d2728ae6915f8e24b74d716 
>   src/slave/flags.cpp d4b4e5201bb3a7e8edd0ab2481328fc91cd8f974 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
> 
> Diff: https://reviews.apache.org/r/43701/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43701: Added a command executor based on the new V1 API.

Posted by Shuai Lin <li...@gmail.com>.

> On Feb. 19, 2016, 4:24 a.m., Shuai Lin wrote:
> > Maybe we should also add a test that launches a task with this new http cmd executor?

ping


- Shuai


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


On Feb. 20, 2016, 1:44 p.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43701/
> -----------------------------------------------------------
> 
> (Updated Feb. 20, 2016, 1:44 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 a command executor based on the new V1 API.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md b04e873009dc783cc50eb02a459f0587d020ad23 
>   include/mesos/v1/mesos.proto e4224afe2245d649afa1a6c97bae26c215e6fada 
>   src/Makefile.am 27aec37524aa33211e0ca4594e127ebb4279e9b0 
>   src/launcher/http_executor.cpp PRE-CREATION 
>   src/slave/flags.hpp 54c1a69d8777f417cdd8f73ce638447d9951ab61 
>   src/slave/flags.cpp 855812e9f7cb4b96d4297f4bd5ac5de7f1d3c39a 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
> 
> Diff: https://reviews.apache.org/r/43701/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43701: Added a command executor based on the new V1 API.

Posted by Shuai Lin <li...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43701/#review119805
-----------------------------------------------------------



Maybe we should also add a test that launches a task with this new http cmd executor?

- Shuai Lin


On Feb. 18, 2016, 1:09 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43701/
> -----------------------------------------------------------
> 
> (Updated Feb. 18, 2016, 1:09 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 a command executor based on the new V1 API.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 3d8236822af688a88a9f9f357c67c03d7d60fdd9 
>   include/mesos/v1/mesos.proto d909e60ddfd8e3ba2075f82c372edde04cd99d54 
>   src/Makefile.am 54ebe91643a17017c00cdbd5dfc8ce1a021579d5 
>   src/launcher/http_executor.cpp PRE-CREATION 
>   src/slave/flags.hpp bd52b4f9816b271a9d2728ae6915f8e24b74d716 
>   src/slave/flags.cpp d4b4e5201bb3a7e8edd0ab2481328fc91cd8f974 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
> 
> Diff: https://reviews.apache.org/r/43701/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43701: Added a command executor based on the new V1 API.

Posted by Qian Zhang <zh...@cn.ibm.com>.

> On Feb. 19, 2016, 12:23 p.m., Shuai Lin wrote:
> > src/launcher/http_executor.cpp, line 776
> > <https://reviews.apache.org/r/43701/diff/1/?file=1253450#file1253450line776>
> >
> >     Unlike unacked status updates, there could be at most one unacked task info, so here we don't need a map of tasks, maybe an `Option<v1::TaskInfo>` is enough.
> >     
> >     I know that the old `mesos::internal::ExecutorProcess` uses a map to store the unacked tasks, but that's because it's part of the generic `ExecutorDriver`, which is designed to support launching multiple tasks with the same executor, while the mesos cmd executor could only launch one task.

Agree! Will fix this soon in the patch.


- Qian


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


On Feb. 18, 2016, 9:09 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43701/
> -----------------------------------------------------------
> 
> (Updated Feb. 18, 2016, 9:09 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 a command executor based on the new V1 API.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 3d8236822af688a88a9f9f357c67c03d7d60fdd9 
>   include/mesos/v1/mesos.proto d909e60ddfd8e3ba2075f82c372edde04cd99d54 
>   src/Makefile.am 54ebe91643a17017c00cdbd5dfc8ce1a021579d5 
>   src/launcher/http_executor.cpp PRE-CREATION 
>   src/slave/flags.hpp bd52b4f9816b271a9d2728ae6915f8e24b74d716 
>   src/slave/flags.cpp d4b4e5201bb3a7e8edd0ab2481328fc91cd8f974 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
> 
> Diff: https://reviews.apache.org/r/43701/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43701: Added a command executor based on the new V1 API.

Posted by Shuai Lin <li...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43701/#review119803
-----------------------------------------------------------




src/launcher/http_executor.cpp (line 776)
<https://reviews.apache.org/r/43701/#comment181154>

    Unlike unacked status updates, there could be at most one unacked task info, so here we don't need a map of tasks, maybe an `Option<v1::TaskInfo>` is enough.
    
    I know that the old `mesos::internal::ExecutorProcess` uses a map to store the unacked tasks, but that's because it's part of the generic `ExecutorDriver`, which is designed to support launching multiple tasks with the same executor, while the mesos cmd executor could only launch one task.


- Shuai Lin


On Feb. 18, 2016, 1:09 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43701/
> -----------------------------------------------------------
> 
> (Updated Feb. 18, 2016, 1:09 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 a command executor based on the new V1 API.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 3d8236822af688a88a9f9f357c67c03d7d60fdd9 
>   include/mesos/v1/mesos.proto d909e60ddfd8e3ba2075f82c372edde04cd99d54 
>   src/Makefile.am 54ebe91643a17017c00cdbd5dfc8ce1a021579d5 
>   src/launcher/http_executor.cpp PRE-CREATION 
>   src/slave/flags.hpp bd52b4f9816b271a9d2728ae6915f8e24b74d716 
>   src/slave/flags.cpp d4b4e5201bb3a7e8edd0ab2481328fc91cd8f974 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
> 
> Diff: https://reviews.apache.org/r/43701/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43701: Added a command executor based on the new V1 API.

Posted by Qian Zhang <zh...@cn.ibm.com>.

> On Feb. 18, 2016, 1:42 p.m., Jian Qiu wrote:
> > src/launcher/http_executor.cpp, line 118
> > <https://reviews.apache.org/r/43701/diff/1/?file=1253450#file1253450line118>
> >
> >     this method and other methods not implementation of the virtual method should be private

I think other executors or schedulers based on V1 HTTP API also do it in the similar way, you can take a look at `EventCallScheduler` and `TestExecutor`.


> On Feb. 18, 2016, 1:42 p.m., Jian Qiu wrote:
> > src/launcher/http_executor.cpp, line 202
> > <https://reviews.apache.org/r/43701/diff/1/?file=1253450#file1253450line202>
> >
> >     should this block be put in a separate function?

Ha, personally I agree! But I just followed the old command executor, you can take a look at `CommandExecutorProcess::launchTask()`, I would like to keep them consistent for now :-)


- Qian


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


On Feb. 18, 2016, 9:09 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43701/
> -----------------------------------------------------------
> 
> (Updated Feb. 18, 2016, 9:09 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 a command executor based on the new V1 API.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 3d8236822af688a88a9f9f357c67c03d7d60fdd9 
>   include/mesos/v1/mesos.proto d909e60ddfd8e3ba2075f82c372edde04cd99d54 
>   src/Makefile.am 54ebe91643a17017c00cdbd5dfc8ce1a021579d5 
>   src/launcher/http_executor.cpp PRE-CREATION 
>   src/slave/flags.hpp bd52b4f9816b271a9d2728ae6915f8e24b74d716 
>   src/slave/flags.cpp d4b4e5201bb3a7e8edd0ab2481328fc91cd8f974 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
> 
> Diff: https://reviews.apache.org/r/43701/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43701: Added a command executor based on the new V1 API.

Posted by Jian Qiu <qi...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43701/#review119584
-----------------------------------------------------------




src/launcher/http_executor.cpp (line 118)
<https://reviews.apache.org/r/43701/#comment180939>

    this method and other methods not implementation of the virtual method should be private



src/launcher/http_executor.cpp (line 202)
<https://reviews.apache.org/r/43701/#comment180940>

    should this block be put in a separate function?


- Jian Qiu


On 二月 18, 2016, 1:09 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43701/
> -----------------------------------------------------------
> 
> (Updated 二月 18, 2016, 1:09 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 a command executor based on the new V1 API.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 3d8236822af688a88a9f9f357c67c03d7d60fdd9 
>   include/mesos/v1/mesos.proto d909e60ddfd8e3ba2075f82c372edde04cd99d54 
>   src/Makefile.am 54ebe91643a17017c00cdbd5dfc8ce1a021579d5 
>   src/launcher/http_executor.cpp PRE-CREATION 
>   src/slave/flags.hpp bd52b4f9816b271a9d2728ae6915f8e24b74d716 
>   src/slave/flags.cpp d4b4e5201bb3a7e8edd0ab2481328fc91cd8f974 
>   src/slave/slave.cpp f0be0d5bf4b853952caf06e2e262c2903d79ead3 
> 
> Diff: https://reviews.apache.org/r/43701/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>