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 2015/09/30 05:39:03 UTC
Review Request 38874: Refactored executor struct in Agent for the
Executor HTTP API
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38874/
-----------------------------------------------------------
Review request for mesos, Ben Mahler, Isabel Jimenez, and Vinod Kone.
Bugs: MESOS-3480
https://issues.apache.org/jira/browse/MESOS-3480
Repository: mesos
Description
-------
This change refactors the Executor struct on Agent and adds support for Executors to connect via the `api/v1/executor` endpoint on Agent. This is similar to the change done in Master for the Scheduler HTTP API.
Diffs
-----
src/slave/slave.hpp 18be4f8188ad34ef4d0aa4b5eba241053d071476
src/slave/slave.cpp d1c9977feeb30ad43586a4560eed155865d27a6c
Diff: https://reviews.apache.org/r/38874/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar
Re: Review Request 38874: Refactored executor struct in Agent for the
Executor HTTP API
Posted by Anand Mazumdar <ma...@gmail.com>.
> On Oct. 5, 2015, 11:51 p.m., Isabel Jimenez wrote:
> > src/slave/slave.hpp, line 42
> > <https://reviews.apache.org/r/38874/diff/1/?file=1087513#file1087513line42>
> >
> > we don't need this
We need this include as the encoder needs it. We can get away with this but that would entail converting the old message to unversioned protobuf and then evolving it again ( 2 conversions ) instead of 1 that is the case now.
The same logic exists in the master too for the same reason. We can revisit this later.
> On Oct. 5, 2015, 11:51 p.m., Isabel Jimenez wrote:
> > src/slave/slave.cpp, line 4054
> > <https://reviews.apache.org/r/38874/diff/1/?file=1087514#file1087514line4054>
> >
> > `executor->pid.isSome() && executor->pid.get() != UPID()` ?
They both mean the same in this context.
https://github.com/apache/mesos/blob/master/3rdparty/libprocess/include/process/pid.hpp#L69
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38874/#review101556
-----------------------------------------------------------
On Oct. 6, 2015, 2:22 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38874/
> -----------------------------------------------------------
>
> (Updated Oct. 6, 2015, 2:22 a.m.)
>
>
> Review request for mesos, Ben Mahler, Isabel Jimenez, and Vinod Kone.
>
>
> Bugs: MESOS-3480
> https://issues.apache.org/jira/browse/MESOS-3480
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change refactors the Executor struct on Agent and adds support for Executors to connect via the `api/v1/executor` endpoint on Agent. This is similar to the change done in Master for the Scheduler HTTP API.
>
>
> Diffs
> -----
>
> src/slave/slave.hpp 18be4f8188ad34ef4d0aa4b5eba241053d071476
> src/slave/slave.cpp d1c9977feeb30ad43586a4560eed155865d27a6c
>
> Diff: https://reviews.apache.org/r/38874/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 38874: Refactored executor struct in Agent for the
Executor HTTP API
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38874/#review101556
-----------------------------------------------------------
src/slave/slave.hpp (line 42)
<https://reviews.apache.org/r/38874/#comment158988>
we don't need this
src/slave/slave.cpp (line 4054)
<https://reviews.apache.org/r/38874/#comment158989>
`executor->pid.isSome() && executor->pid.get() != UPID()` ?
src/slave/slave.cpp (line 4069)
<https://reviews.apache.org/r/38874/#comment158990>
same as above
src/slave/slave.cpp (line 5340)
<https://reviews.apache.org/r/38874/#comment158992>
same as above
- Isabel Jimenez
On Sept. 30, 2015, 3:39 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38874/
> -----------------------------------------------------------
>
> (Updated Sept. 30, 2015, 3:39 a.m.)
>
>
> Review request for mesos, Ben Mahler, Isabel Jimenez, and Vinod Kone.
>
>
> Bugs: MESOS-3480
> https://issues.apache.org/jira/browse/MESOS-3480
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change refactors the Executor struct on Agent and adds support for Executors to connect via the `api/v1/executor` endpoint on Agent. This is similar to the change done in Master for the Scheduler HTTP API.
>
>
> Diffs
> -----
>
> src/slave/slave.hpp 18be4f8188ad34ef4d0aa4b5eba241053d071476
> src/slave/slave.cpp d1c9977feeb30ad43586a4560eed155865d27a6c
>
> Diff: https://reviews.apache.org/r/38874/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 38874: Refactored executor struct in Agent for the
Executor HTTP API
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38874/#review105765
-----------------------------------------------------------
src/slave/slave.hpp (line 679)
<https://reviews.apache.org/r/38874/#comment164399>
I would avoid using 'Agent' for now in the code (except the v1 API). Constantly switching between 'agent' and 'slave' is not good for readability.
src/slave/slave.cpp (line 5166)
<https://reviews.apache.org/r/38874/#comment164397>
Do you want to change that as well?
- Jie Yu
On Oct. 23, 2015, 1:34 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38874/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2015, 1:34 a.m.)
>
>
> Review request for mesos, Ben Mahler, Isabel Jimenez, and Vinod Kone.
>
>
> Bugs: MESOS-3480
> https://issues.apache.org/jira/browse/MESOS-3480
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change refactors the Executor struct on Agent and adds support for Executors to connect via the `api/v1/executor` endpoint on Agent. This is similar to the change done in Master for the Scheduler HTTP API.
>
>
> Diffs
> -----
>
> src/slave/slave.hpp d81a9c4d09d3f6be417120e412324258f19604a3
> src/slave/slave.cpp e9f2d1bf7978450f0cd471bdb5606305092fc98a
>
> Diff: https://reviews.apache.org/r/38874/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 38874: Refactored executor struct in Agent for the
Executor HTTP API
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38874/#review103723
-----------------------------------------------------------
Ship it!
Thanks for the patience and for reworking the patches!
I think the CHECK_SOME in Executor::send is problematic, see below. I made some adjustments based on my comments in the review, please have a look over the commit diff!
src/slave/slave.hpp (line 637)
<https://reviews.apache.org/r/38874/#comment161793>
Doesn't look like this should be a CHECK_SOME given the possibility of both being None, mind just logging a warning when we drop the message instead of the CHECK_SOME?
src/slave/slave.hpp (line 685)
<https://reviews.apache.org/r/38874/#comment161794>
How about s/Unknown/Not known yet/ to suggest that we find out later?
src/slave/slave.hpp
<https://reviews.apache.org/r/38874/#comment161795>
Hm.. doesn't this still need to be in the header? How do the output operations in Executor above compile?
src/slave/slave.cpp (line 4114)
<https://reviews.apache.org/r/38874/#comment161796>
It looks like this call can trip the CHECK_SOME in Executor::send? :(
src/slave/slave.cpp (line 5399)
<https://reviews.apache.org/r/38874/#comment161797>
The "at IP:PORT" should be sufficient, no need to say of type PID.
src/slave/slave.cpp (line 5404)
<https://reviews.apache.org/r/38874/#comment161799>
how about "(via HTTP)"
src/slave/slave.cpp (line 5406)
<https://reviews.apache.org/r/38874/#comment161798>
I think this will come out confusing in the logs, if I just read "of type: Unknown" it's not clear what the meaning of type is here. Would suggest saying nothing here.
- Ben Mahler
On Oct. 23, 2015, 1:34 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38874/
> -----------------------------------------------------------
>
> (Updated Oct. 23, 2015, 1:34 a.m.)
>
>
> Review request for mesos, Ben Mahler, Isabel Jimenez, and Vinod Kone.
>
>
> Bugs: MESOS-3480
> https://issues.apache.org/jira/browse/MESOS-3480
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change refactors the Executor struct on Agent and adds support for Executors to connect via the `api/v1/executor` endpoint on Agent. This is similar to the change done in Master for the Scheduler HTTP API.
>
>
> Diffs
> -----
>
> src/slave/slave.hpp d81a9c4d09d3f6be417120e412324258f19604a3
> src/slave/slave.cpp e9f2d1bf7978450f0cd471bdb5606305092fc98a
>
> Diff: https://reviews.apache.org/r/38874/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 38874: Refactored executor struct in Agent for the
Executor HTTP API
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38874/
-----------------------------------------------------------
(Updated Oct. 23, 2015, 1:34 a.m.)
Review request for mesos, Ben Mahler, Isabel Jimenez, and Vinod Kone.
Changes
-------
Rebased
Bugs: MESOS-3480
https://issues.apache.org/jira/browse/MESOS-3480
Repository: mesos
Description
-------
This change refactors the Executor struct on Agent and adds support for Executors to connect via the `api/v1/executor` endpoint on Agent. This is similar to the change done in Master for the Scheduler HTTP API.
Diffs (updated)
-----
src/slave/slave.hpp d81a9c4d09d3f6be417120e412324258f19604a3
src/slave/slave.cpp e9f2d1bf7978450f0cd471bdb5606305092fc98a
Diff: https://reviews.apache.org/r/38874/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar
Re: Review Request 38874: Refactored executor struct in Agent for the
Executor HTTP API
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38874/
-----------------------------------------------------------
(Updated Oct. 22, 2015, 8:46 p.m.)
Review request for mesos, Ben Mahler, Isabel Jimenez, and Vinod Kone.
Changes
-------
Updated depends on and moved adding ostream for executor to a separate patch
Bugs: MESOS-3480
https://issues.apache.org/jira/browse/MESOS-3480
Repository: mesos
Description
-------
This change refactors the Executor struct on Agent and adds support for Executors to connect via the `api/v1/executor` endpoint on Agent. This is similar to the change done in Master for the Scheduler HTTP API.
Diffs (updated)
-----
src/slave/slave.hpp d81a9c4d09d3f6be417120e412324258f19604a3
src/slave/slave.cpp e9f2d1bf7978450f0cd471bdb5606305092fc98a
Diff: https://reviews.apache.org/r/38874/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar
Re: Review Request 38874: Refactored executor struct in Agent for the
Executor HTTP API
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38874/
-----------------------------------------------------------
(Updated Oct. 22, 2015, 5:51 a.m.)
Review request for mesos, Ben Mahler, Isabel Jimenez, and Vinod Kone.
Changes
-------
Applied BenM's diff + some minor changes in the ostream operator as per review comments
Bugs: MESOS-3480
https://issues.apache.org/jira/browse/MESOS-3480
Repository: mesos
Description
-------
This change refactors the Executor struct on Agent and adds support for Executors to connect via the `api/v1/executor` endpoint on Agent. This is similar to the change done in Master for the Scheduler HTTP API.
Diffs (updated)
-----
src/slave/slave.hpp d6e417b1ddde44a557e1e7fb3d22b4fd5bd0d526
src/slave/slave.cpp cb2d715d100bcfc76d80663ad13504bd278f1200
Diff: https://reviews.apache.org/r/38874/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar
Re: Review Request 38874: Refactored executor struct in Agent for the
Executor HTTP API
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38874/
-----------------------------------------------------------
(Updated Oct. 14, 2015, 12:08 a.m.)
Review request for mesos, Ben Mahler, Isabel Jimenez, and Vinod Kone.
Changes
-------
Review comments from Ben
Bugs: MESOS-3480
https://issues.apache.org/jira/browse/MESOS-3480
Repository: mesos
Description
-------
This change refactors the Executor struct on Agent and adds support for Executors to connect via the `api/v1/executor` endpoint on Agent. This is similar to the change done in Master for the Scheduler HTTP API.
Diffs (updated)
-----
src/slave/slave.hpp d6e417b1ddde44a557e1e7fb3d22b4fd5bd0d526
src/slave/slave.cpp cb2d715d100bcfc76d80663ad13504bd278f1200
Diff: https://reviews.apache.org/r/38874/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar
Re: Review Request 38874: Refactored executor struct in Agent for the
Executor HTTP API
Posted by Anand Mazumdar <ma...@gmail.com>.
> On Oct. 7, 2015, 11:31 p.m., Ben Mahler wrote:
> > src/slave/slave.hpp, line 583
> > <https://reviews.apache.org/r/38874/diff/2/?file=1091510#file1091510line583>
> >
> > Could you add the closed() method here to make this consistent with the scheduler's HttpConnection struct in the master? We may want to consolidate these two structs later.
Done
> On Oct. 7, 2015, 11:31 p.m., Ben Mahler wrote:
> > src/slave/slave.hpp, lines 622-625
> > <https://reviews.apache.org/r/38874/diff/2/?file=1091510#file1091510line622>
> >
> > Looks like you also want to warn about TERMINATING?
> >
> > How about just printing the state instead of saying "disconnected" to be a bit more explicit?
When the executor is launched, the agent sets its `state = REGISTERING`. So it looks fine to have the `state == REGISTERING` check.
However, what I had missed was, when the agent sends an executor a `Shutdown` event, it marks it's state as `TERMINATING` and then sends the event. In short, we would always be disconnected when the states are `TERMINATED/REGISTERING`. I added the state to the `LOG(WARNING)`. Thanks for the suggestion.
> On Oct. 7, 2015, 11:31 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, lines 5117-5119
> > <https://reviews.apache.org/r/38874/diff/2/?file=1091511#file1091511line5117>
> >
> > Per our chat and my other suggestion, hoping we can turn this case into neither 'pid' nor 'http' being set.
Fixed
> On Oct. 7, 2015, 11:31 p.m., Ben Mahler wrote:
> > src/slave/slave.hpp, line 671
> > <https://reviews.apache.org/r/38874/diff/2/?file=1091510#file1091510line671>
> >
> > Does this first "when" belong? It seems a bit confusing that we have this pid = UPID() case, ideally we can be explicit about when we don't know the connection type yet by having both set to None, i.e. the equivalent of:
> >
> > ```
> > Either<None, UPID, HttpConnection>
> > ```
As we had discussed offline, `PID/HTTP` would be both `None()` on launch. While recovering, if the agent is not able to find the `pid/http` marker file it would set `pid = UPID()`.
> On Oct. 7, 2015, 11:31 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 2391
> > <https://reviews.apache.org/r/38874/diff/2/?file=1091511#file1091511line2391>
> >
> > Ah, I meant to remove `reply` completely due to it being error prone (the implicit use of a UPID means it breaks if called asynchronously), see:
> >
> > https://issues.apache.org/jira/browse/MESOS-765
> > Here's a case of it causing a bug: https://issues.apache.org/jira/browse/MESOS-1310
> >
> > We should update these in a separate patch to do explicit sending based on `from` and remove `reply` completely.
Sending in a patch for these shortly :)
> On Oct. 7, 2015, 11:31 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 2840
> > <https://reviews.apache.org/r/38874/diff/2/?file=1091511#file1091511line2840>
> >
> > It's a little weird to have these pid checks down here.
> >
> > I was going to suggest having a CHECK_SOME at the top of each message handler, but I'm not sure that's safe (can these paths get triggered when 'pid' is none?).
Previously, we supported other executors to send `statusUpdates` on behalf of other executors as you had pointed out. If the other executor has not yet `registered`, the value of `executor->pid` would still be `None()` for it. So , I had to include these extra `isSome()` checks since these paths can get triggered when `pid` is None()
> On Oct. 7, 2015, 11:31 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, lines 2850-2851
> > <https://reviews.apache.org/r/38874/diff/2/?file=1091511#file1091511line2850>
> >
> > Just for some context, we originally added this for Aurora's GC executor, which has now been obsoleted by reconciliation and is no longer in use.
> >
> > With the HTTP api, this gets tricker to support (we can't use PIDs and have to use session identifiers for this). Given that, I'm not sure if we should continue supporting it in the HTTP code paths.
Based on my conversations with Vinod + what we had already discussed, we won't be supporting the ability of `HTTP` executors to send messages on behalf of others. Hopefully, that should make the code-paths in `MESOS-3476` much simpler.
> On Oct. 7, 2015, 11:31 p.m., Ben Mahler wrote:
> > src/slave/slave.hpp, lines 668-669
> > <https://reviews.apache.org/r/38874/diff/2/?file=1091510#file1091510line668>
> >
> > "libprocess message passing"
Fixed
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38874/#review101830
-----------------------------------------------------------
On Oct. 6, 2015, 2:22 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38874/
> -----------------------------------------------------------
>
> (Updated Oct. 6, 2015, 2:22 a.m.)
>
>
> Review request for mesos, Ben Mahler, Isabel Jimenez, and Vinod Kone.
>
>
> Bugs: MESOS-3480
> https://issues.apache.org/jira/browse/MESOS-3480
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change refactors the Executor struct on Agent and adds support for Executors to connect via the `api/v1/executor` endpoint on Agent. This is similar to the change done in Master for the Scheduler HTTP API.
>
>
> Diffs
> -----
>
> src/slave/slave.hpp 18be4f8188ad34ef4d0aa4b5eba241053d071476
> src/slave/slave.cpp d1c9977feeb30ad43586a4560eed155865d27a6c
>
> Diff: https://reviews.apache.org/r/38874/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 38874: Refactored executor struct in Agent for the
Executor HTTP API
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38874/#review101830
-----------------------------------------------------------
Looks good, thanks for the small review! Main thing is I'm curious if we can avoid the UPID() case for 'pid'.
src/slave/slave.hpp (line 583)
<https://reviews.apache.org/r/38874/#comment159329>
Could you add the closed() method here to make this consistent with the scheduler's HttpConnection struct in the master? We may want to consolidate these two structs later.
src/slave/slave.hpp (lines 622 - 625)
<https://reviews.apache.org/r/38874/#comment159330>
Looks like you also want to warn about TERMINATING?
How about just printing the state instead of saying "disconnected" to be a bit more explicit?
src/slave/slave.hpp (lines 668 - 669)
<https://reviews.apache.org/r/38874/#comment159331>
"libprocess message passing"
src/slave/slave.hpp (line 671)
<https://reviews.apache.org/r/38874/#comment159332>
Does this first "when" belong? It seems a bit confusing that we have this pid = UPID() case, ideally we can be explicit about when we don't know the connection type yet by having both set to None, i.e. the equivalent of:
```
Either<None, UPID, HttpConnection>
```
src/slave/slave.cpp (line 2391)
<https://reviews.apache.org/r/38874/#comment159339>
Ah, I meant to remove `reply` completely due to it being error prone (the implicit use of a UPID means it breaks if called asynchronously), see:
https://issues.apache.org/jira/browse/MESOS-765
Here's a case of it causing a bug: https://issues.apache.org/jira/browse/MESOS-1310
We should update these in a separate patch to do explicit sending based on `from` and remove `reply` completely.
src/slave/slave.cpp (line 2840)
<https://reviews.apache.org/r/38874/#comment159338>
It's a little weird to have these pid checks down here.
I was going to suggest having a CHECK_SOME at the top of each message handler, but I'm not sure that's safe (can these paths get triggered when 'pid' is none?).
src/slave/slave.cpp (lines 2850 - 2851)
<https://reviews.apache.org/r/38874/#comment159335>
Just for some context, we originally added this for Aurora's GC executor, which has now been obsoleted by reconciliation and is no longer in use.
With the HTTP api, this gets tricker to support (we can't use PIDs and have to use session identifiers for this). Given that, I'm not sure if we should continue supporting it in the HTTP code paths.
src/slave/slave.cpp (lines 5117 - 5119)
<https://reviews.apache.org/r/38874/#comment159334>
Per our chat and my other suggestion, hoping we can turn this case into neither 'pid' nor 'http' being set.
- Ben Mahler
On Oct. 6, 2015, 2:22 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38874/
> -----------------------------------------------------------
>
> (Updated Oct. 6, 2015, 2:22 a.m.)
>
>
> Review request for mesos, Ben Mahler, Isabel Jimenez, and Vinod Kone.
>
>
> Bugs: MESOS-3480
> https://issues.apache.org/jira/browse/MESOS-3480
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change refactors the Executor struct on Agent and adds support for Executors to connect via the `api/v1/executor` endpoint on Agent. This is similar to the change done in Master for the Scheduler HTTP API.
>
>
> Diffs
> -----
>
> src/slave/slave.hpp 18be4f8188ad34ef4d0aa4b5eba241053d071476
> src/slave/slave.cpp d1c9977feeb30ad43586a4560eed155865d27a6c
>
> Diff: https://reviews.apache.org/r/38874/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 38874: Refactored executor struct in Agent for the
Executor HTTP API
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38874/
-----------------------------------------------------------
(Updated Oct. 6, 2015, 2:22 a.m.)
Review request for mesos, Ben Mahler, Isabel Jimenez, and Vinod Kone.
Changes
-------
Review comments
Bugs: MESOS-3480
https://issues.apache.org/jira/browse/MESOS-3480
Repository: mesos
Description
-------
This change refactors the Executor struct on Agent and adds support for Executors to connect via the `api/v1/executor` endpoint on Agent. This is similar to the change done in Master for the Scheduler HTTP API.
Diffs (updated)
-----
src/slave/slave.hpp 18be4f8188ad34ef4d0aa4b5eba241053d071476
src/slave/slave.cpp d1c9977feeb30ad43586a4560eed155865d27a6c
Diff: https://reviews.apache.org/r/38874/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar
Re: Review Request 38874: Refactored executor struct in Agent for the
Executor HTTP API
Posted by Anand Mazumdar <ma...@gmail.com>.
> On Sept. 30, 2015, 4:25 a.m., Guangya Liu wrote:
> > src/slave/slave.cpp, line 5325
> > <https://reviews.apache.org/r/38874/diff/1/?file=1087514#file1087514line5325>
> >
> > Can you please add some comments here? Just curious why here the state is REGISTERING? Thanks.
Ideally this would have been an invariant check i.e. CHECK(state != REGISTERING) but it seems that the PID based executor workflow does not do it too. For consitency between the two removing this.
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38874/#review101071
-----------------------------------------------------------
On Oct. 6, 2015, 2:22 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38874/
> -----------------------------------------------------------
>
> (Updated Oct. 6, 2015, 2:22 a.m.)
>
>
> Review request for mesos, Ben Mahler, Isabel Jimenez, and Vinod Kone.
>
>
> Bugs: MESOS-3480
> https://issues.apache.org/jira/browse/MESOS-3480
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change refactors the Executor struct on Agent and adds support for Executors to connect via the `api/v1/executor` endpoint on Agent. This is similar to the change done in Master for the Scheduler HTTP API.
>
>
> Diffs
> -----
>
> src/slave/slave.hpp 18be4f8188ad34ef4d0aa4b5eba241053d071476
> src/slave/slave.cpp d1c9977feeb30ad43586a4560eed155865d27a6c
>
> Diff: https://reviews.apache.org/r/38874/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 38874: Refactored executor struct in Agent for the
Executor HTTP API
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38874/#review101071
-----------------------------------------------------------
src/slave/slave.cpp (line 5325)
<https://reviews.apache.org/r/38874/#comment158379>
Can you please add some comments here? Just curious why here the state is REGISTERING? Thanks.
- Guangya Liu
On 九月 30, 2015, 3:39 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38874/
> -----------------------------------------------------------
>
> (Updated 九月 30, 2015, 3:39 a.m.)
>
>
> Review request for mesos, Ben Mahler, Isabel Jimenez, and Vinod Kone.
>
>
> Bugs: MESOS-3480
> https://issues.apache.org/jira/browse/MESOS-3480
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change refactors the Executor struct on Agent and adds support for Executors to connect via the `api/v1/executor` endpoint on Agent. This is similar to the change done in Master for the Scheduler HTTP API.
>
>
> Diffs
> -----
>
> src/slave/slave.hpp 18be4f8188ad34ef4d0aa4b5eba241053d071476
> src/slave/slave.cpp d1c9977feeb30ad43586a4560eed155865d27a6c
>
> Diff: https://reviews.apache.org/r/38874/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Anand Mazumdar
>
>