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



src/executor/executor.cpp (lines 20 - 21)
<https://reviews.apache.org/r/41283/#comment175055>

    reorder alphabetically.



src/executor/executor.cpp (line 123)
<https://reviews.apache.org/r/41283/#comment175123>

    The name of this struct is weird considering it is encapsulating two 
connections.
    
    s/HttpConnection/connections/ ?
    
    Also, do we really need a struct?



src/executor/executor.cpp (line 131)
<https://reviews.apache.org/r/41283/#comment175056>

    s/'Other'/other/
    
    also `other` seems to be a weird name for the variable. how about calling 
it `nonSubscribe`?



src/executor/executor.cpp (lines 179 - 183)
<https://reviews.apache.org/r/41283/#comment175062>

    is mesos_slave_pid the only way an executor is going to know about the http 
endpoint to connect? i think we need to have a better env variable because 
'pid' is a relic of the old world, which doesn't make sense in the http world.
    
    wasn't there an MESOS_AGENT_ENDPOINT?



src/executor/executor.cpp (line 205)
<https://reviews.apache.org/r/41283/#comment175065>

    s/Cannot/Failed to/



src/executor/executor.cpp (line 220)
<https://reviews.apache.org/r/41283/#comment175066>

    s/Cannot/Failed to/



src/executor/executor.cpp (lines 282 - 287)
<https://reviews.apache.org/r/41283/#comment175428>

    This definitely needs a comment here. IIUC, you are opening up two 
connections here one for subscribe and another for everything else.
    
    More importantly, why are you opening both connections here? I would've 
expected to only open the subscribe connection if it's a subscribe calla and 
open a non-subscribe one if/when it's a nonsubscribe call.



src/executor/executor.cpp (lines 309 - 313)
<https://reviews.apache.org/r/41283/#comment175429>

    I dont think the executor need to be informed if this connection failed. In 
the design doc we said that subscription connection is the only one the server 
(master/agent) keeps track of. For example thats the only connection master 
sends HEARTBEATS on.



src/executor/executor.cpp (line 334)
<https://reviews.apache.org/r/41283/#comment175431>

    why do we wait for `recoveryTimeout` if the agent is disconnected and we 
are not checkpointing?



src/executor/executor.cpp (line 345)
<https://reviews.apache.org/r/41283/#comment175432>

    This seems like a comment for the else block.
    
    How about
    
    // Backoff and reconnect if checkpointing is enabled.



src/executor/executor.cpp (line 382)
<https://reviews.apache.org/r/41283/#comment175433>

    s/been/seen/ ?



src/executor/executor.cpp (line 415)
<https://reviews.apache.org/r/41283/#comment175434>

    s/causing/caused/


- Vinod Kone


On Jan. 12, 2016, 9:21 a.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41283/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2016, 9:21 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-3550
>     https://issues.apache.org/jira/browse/MESOS-3550
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This change introduces the implementation for the executor library. 
> 
> This uses the new HTTP Connection interface to ensure calls are properly 
> pipelined.
> 
> `connected` -> Callback invoked when a TCP connection is established with the 
> agent.
> `disconnected` -> When the TCP connection is interrupted possibly due to an 
> agent restart.
> `received` -> When the executor receives events from the agent upon 
> subscribing.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 8cbfb1ba5fa49f2d3cc26ea325838a1c68a79660 
>   src/executor/executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41283/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Reply via email to