You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Isabel Jimenez <co...@isabeljimenez.com> on 2015/09/05 04:35:50 UTC
Review Request 38143: Adding executor HTTP API protobuf to V1
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38143/
-----------------------------------------------------------
Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
Bugs: MESOS-3375
https://issues.apache.org/jira/browse/MESOS-3375
Repository: mesos
Description
-------
Executor protobufswere introduced in Mesos for the HTTP API, they need to be added to /v1 so they reflect changes made on v1/mesos.proto. This protobuf ought to be changed as the executor HTTP API design evolves.
Diffs
-----
include/mesos/v1/executor/executor.hpp PRE-CREATION
include/mesos/v1/executor/executor.proto PRE-CREATION
src/Makefile.am 5fdca0f
Diff: https://reviews.apache.org/r/38143/diff/
Testing
-------
make && make check
Thanks,
Isabel Jimenez
Re: Review Request 38143: Adding executor HTTP API protobuf to V1
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38143/#review97876
-----------------------------------------------------------
Patch looks great!
Reviews applied: [38143]
All tests passed.
- Mesos ReviewBot
On Sept. 5, 2015, 10:05 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38143/
> -----------------------------------------------------------
>
> (Updated Sept. 5, 2015, 10:05 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-3375
> https://issues.apache.org/jira/browse/MESOS-3375
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Executor protobufswere introduced in Mesos for the HTTP API, they need to be added to /v1 so they reflect changes made on v1/mesos.proto. This protobuf ought to be changed as the executor HTTP API design evolves.
>
>
> Diffs
> -----
>
> include/mesos/v1/executor/executor.hpp PRE-CREATION
> include/mesos/v1/executor/executor.proto PRE-CREATION
> src/Makefile.am 5fdca0f
>
> Diff: https://reviews.apache.org/r/38143/diff/
>
>
> Testing
> -------
>
> make && make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 38143: Adding executor HTTP API protobuf to V1
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38143/#review98047
-----------------------------------------------------------
Ship it!
LGTM !
@gyliu : This change just moves the existing executor protobuf's to the V1 namespace, we can revisit the semantics later when we finalize the design doc. Also, thanks for pointing out the typos we had missed during the earlier review of these protobuf's.
@ijimenez : Can we delete the old executor protobuf's/headers in a separate patch ? I am assuming that no one is using them.
- Anand Mazumdar
On Sept. 5, 2015, 10:05 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38143/
> -----------------------------------------------------------
>
> (Updated Sept. 5, 2015, 10:05 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-3375
> https://issues.apache.org/jira/browse/MESOS-3375
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Executor protobufswere introduced in Mesos for the HTTP API, they need to be added to /v1 so they reflect changes made on v1/mesos.proto. This protobuf ought to be changed as the executor HTTP API design evolves.
>
>
> Diffs
> -----
>
> include/mesos/v1/executor/executor.hpp PRE-CREATION
> include/mesos/v1/executor/executor.proto PRE-CREATION
> src/Makefile.am 5fdca0f
>
> Diff: https://reviews.apache.org/r/38143/diff/
>
>
> Testing
> -------
>
> make && make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 38143: Adding executor HTTP API protobuf to V1
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38143/#review97880
-----------------------------------------------------------
Ship it!
Thanks Isabel for the explanation!
- Guangya Liu
On 九月 5, 2015, 10:05 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38143/
> -----------------------------------------------------------
>
> (Updated 九月 5, 2015, 10:05 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-3375
> https://issues.apache.org/jira/browse/MESOS-3375
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Executor protobufswere introduced in Mesos for the HTTP API, they need to be added to /v1 so they reflect changes made on v1/mesos.proto. This protobuf ought to be changed as the executor HTTP API design evolves.
>
>
> Diffs
> -----
>
> include/mesos/v1/executor/executor.hpp PRE-CREATION
> include/mesos/v1/executor/executor.proto PRE-CREATION
> src/Makefile.am 5fdca0f
>
> Diff: https://reviews.apache.org/r/38143/diff/
>
>
> Testing
> -------
>
> make && make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 38143: Adding executor HTTP API protobuf to V1
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38143/
-----------------------------------------------------------
(Updated Sept. 5, 2015, 10:05 p.m.)
Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
Changes
-------
Fixing typo
Bugs: MESOS-3375
https://issues.apache.org/jira/browse/MESOS-3375
Repository: mesos
Description
-------
Executor protobufswere introduced in Mesos for the HTTP API, they need to be added to /v1 so they reflect changes made on v1/mesos.proto. This protobuf ought to be changed as the executor HTTP API design evolves.
Diffs (updated)
-----
include/mesos/v1/executor/executor.hpp PRE-CREATION
include/mesos/v1/executor/executor.proto PRE-CREATION
src/Makefile.am 5fdca0f
Diff: https://reviews.apache.org/r/38143/diff/
Testing
-------
make && make check
Thanks,
Isabel Jimenez
Re: Review Request 38143: Adding executor HTTP API protobuf to V1
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38143/#review97847
-----------------------------------------------------------
Patch looks great!
Reviews applied: [38143]
All tests passed.
- Mesos ReviewBot
On Sept. 5, 2015, 2:35 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38143/
> -----------------------------------------------------------
>
> (Updated Sept. 5, 2015, 2:35 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-3375
> https://issues.apache.org/jira/browse/MESOS-3375
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Executor protobufswere introduced in Mesos for the HTTP API, they need to be added to /v1 so they reflect changes made on v1/mesos.proto. This protobuf ought to be changed as the executor HTTP API design evolves.
>
>
> Diffs
> -----
>
> include/mesos/v1/executor/executor.hpp PRE-CREATION
> include/mesos/v1/executor/executor.proto PRE-CREATION
> src/Makefile.am 5fdca0f
>
> Diff: https://reviews.apache.org/r/38143/diff/
>
>
> Testing
> -------
>
> make && make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 38143: Adding executor HTTP API protobuf to V1
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38143/#review97872
-----------------------------------------------------------
include/mesos/v1/executor/executor.proto (line 47)
<https://reviews.apache.org/r/38143/#comment153932>
Here we're refering to 'Events', for the Subscribed event message FrameworkInfo will have the framework_id. Please note that this protobuf ought to have minor changes as the design evolves:
https://docs.google.com/document/d/1dFmTrSZXCo5zj8H8SkJ4HT-V0z2YYnEZVV8Fd_-AupM/edit?usp=sharing
include/mesos/v1/executor/executor.proto (line 91)
<https://reviews.apache.org/r/38143/#comment153933>
arojas is the orginal author of this TODO, if the assignee for this TODO needs to be rewritten, we should do it in a separate patch.
include/mesos/v1/executor/executor.proto (line 169)
<https://reviews.apache.org/r/38143/#comment153934>
The design here follows the design for the scheduler HTTP API. All event messages require their type. Depending on the 'type' of call message, you should expect an optional corresponding type of message.
For example if you have a 'Call' message of 'type' 'UPDATE' your call message will have the optional 'update' message inside with their required 'status' , 'timestamp' and 'uuid'.
- Isabel Jimenez
On Sept. 5, 2015, 2:35 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38143/
> -----------------------------------------------------------
>
> (Updated Sept. 5, 2015, 2:35 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-3375
> https://issues.apache.org/jira/browse/MESOS-3375
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Executor protobufswere introduced in Mesos for the HTTP API, they need to be added to /v1 so they reflect changes made on v1/mesos.proto. This protobuf ought to be changed as the executor HTTP API design evolves.
>
>
> Diffs
> -----
>
> include/mesos/v1/executor/executor.hpp PRE-CREATION
> include/mesos/v1/executor/executor.proto PRE-CREATION
> src/Makefile.am 5fdca0f
>
> Diff: https://reviews.apache.org/r/38143/diff/
>
>
> Testing
> -------
>
> make && make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 38143: Adding executor HTTP API protobuf to V1
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
> On Sept. 5, 2015, 3:28 a.m., Guangya Liu wrote:
> > include/mesos/v1/executor/executor.proto, line 47
> > <https://reviews.apache.org/r/38143/diff/1/?file=1064550#file1064550line47>
> >
> > I think that the framework id need to be set when call "SUBSCRIBE" but not after "SUBSCRIBED"?
Here we're refering to 'Events', for the Subscribed event message FrameworkInfo will have the framework_id. Please note that this protobuf ought to have minor changes as the design evolves:
https://docs.google.com/document/d/1dFmTrSZXCo5zj8H8SkJ4HT-V0z2YYnEZVV8Fd_-AupM/edit?usp=sharing
> On Sept. 5, 2015, 3:28 a.m., Guangya Liu wrote:
> > include/mesos/v1/executor/executor.proto, line 91
> > <https://reviews.apache.org/r/38143/diff/1/?file=1064550#file1064550line91>
> >
> > s/arojas/yourid?
arojas is the orginal author of this TODO, if the assignee for this TODO needs to be rewritten, we should do it in a separate patch.
> On Sept. 5, 2015, 3:28 a.m., Guangya Liu wrote:
> > include/mesos/v1/executor/executor.proto, line 169
> > <https://reviews.apache.org/r/38143/diff/1/?file=1064550#file1064550line169>
> >
> > Do you mean that when type is Subscribe, there is no need to set any message? Why? I think that the framework id, executor id, command should be set?
The design here follows the design for the scheduler HTTP API. All call and event messages require their type. Depending on the 'type' of call message, you should expect an optional corresponding type of message.
For example if you have a 'Call' message of 'type' 'UPDATE' your call message will have the optional 'update' message inside with their required 'status' , 'timestamp' and 'uuid'.
- Isabel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38143/#review97846
-----------------------------------------------------------
On Sept. 5, 2015, 2:35 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38143/
> -----------------------------------------------------------
>
> (Updated Sept. 5, 2015, 2:35 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-3375
> https://issues.apache.org/jira/browse/MESOS-3375
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Executor protobufswere introduced in Mesos for the HTTP API, they need to be added to /v1 so they reflect changes made on v1/mesos.proto. This protobuf ought to be changed as the executor HTTP API design evolves.
>
>
> Diffs
> -----
>
> include/mesos/v1/executor/executor.hpp PRE-CREATION
> include/mesos/v1/executor/executor.proto PRE-CREATION
> src/Makefile.am 5fdca0f
>
> Diff: https://reviews.apache.org/r/38143/diff/
>
>
> Testing
> -------
>
> make && make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 38143: Adding executor HTTP API protobuf to V1
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
> On Sept. 5, 2015, 3:28 a.m., Guangya Liu wrote:
> > include/mesos/v1/executor/executor.proto, line 169
> > <https://reviews.apache.org/r/38143/diff/1/?file=1064550#file1064550line169>
> >
> > Do you mean that when type is Subscribe, there is no need to set any message? Why? I think that the framework id, executor id, command should be set?
>
> Isabel Jimenez wrote:
> The design here follows the design for the scheduler HTTP API. All call and event messages require their type. Depending on the 'type' of call message, you should expect an optional corresponding type of message.
> For example if you have a 'Call' message of 'type' 'UPDATE' your call message will have the optional 'update' message inside with their required 'status' , 'timestamp' and 'uuid'.
Here you can find the design doc for the scheduler HTTP API: https://docs.google.com/document/d/1pnIY_HckimKNvpqhKRhbc9eSItWNFT-priXh_urR-T0/edit?usp=sharing
- Isabel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38143/#review97846
-----------------------------------------------------------
On Sept. 5, 2015, 10:05 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38143/
> -----------------------------------------------------------
>
> (Updated Sept. 5, 2015, 10:05 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-3375
> https://issues.apache.org/jira/browse/MESOS-3375
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Executor protobufswere introduced in Mesos for the HTTP API, they need to be added to /v1 so they reflect changes made on v1/mesos.proto. This protobuf ought to be changed as the executor HTTP API design evolves.
>
>
> Diffs
> -----
>
> include/mesos/v1/executor/executor.hpp PRE-CREATION
> include/mesos/v1/executor/executor.proto PRE-CREATION
> src/Makefile.am 5fdca0f
>
> Diff: https://reviews.apache.org/r/38143/diff/
>
>
> Testing
> -------
>
> make && make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 38143: Adding executor HTTP API protobuf to V1
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38143/#review97846
-----------------------------------------------------------
include/mesos/v1/executor/executor.proto (line 47)
<https://reviews.apache.org/r/38143/#comment153914>
I think that the framework id need to be set when call "SUBSCRIBE" but not after "SUBSCRIBED"?
include/mesos/v1/executor/executor.proto (line 55)
<https://reviews.apache.org/r/38143/#comment153917>
s/successfuly/successfully
include/mesos/v1/executor/executor.proto (line 91)
<https://reviews.apache.org/r/38143/#comment153918>
s/arojas/yourid?
include/mesos/v1/executor/executor.proto (line 169)
<https://reviews.apache.org/r/38143/#comment153919>
Do you mean that when type is Subscribe, there is no need to set any message? Why? I think that the framework id, executor id, command should be set?
- Guangya Liu
On 九月 5, 2015, 2:35 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38143/
> -----------------------------------------------------------
>
> (Updated 九月 5, 2015, 2:35 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-3375
> https://issues.apache.org/jira/browse/MESOS-3375
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Executor protobufswere introduced in Mesos for the HTTP API, they need to be added to /v1 so they reflect changes made on v1/mesos.proto. This protobuf ought to be changed as the executor HTTP API design evolves.
>
>
> Diffs
> -----
>
> include/mesos/v1/executor/executor.hpp PRE-CREATION
> include/mesos/v1/executor/executor.proto PRE-CREATION
> src/Makefile.am 5fdca0f
>
> Diff: https://reviews.apache.org/r/38143/diff/
>
>
> Testing
> -------
>
> make && make check
>
>
> Thanks,
>
> Isabel Jimenez
>
>