-----------------------------------------------------------
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
> 
>

Reply via email to