This is an automatically generated e-mail. To reply, visit:

src/executor/executor.cpp (lines 20 - 21)

    reorder alphabetically.

src/executor/executor.cpp (line 123)

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

src/executor/executor.cpp (line 131)

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

src/executor/executor.cpp (lines 179 - 183)

    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)

    s/Cannot/Failed to/

src/executor/executor.cpp (line 220)

    s/Cannot/Failed to/

src/executor/executor.cpp (lines 282 - 287)

    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)

    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)

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

src/executor/executor.cpp (line 345)

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

src/executor/executor.cpp (line 382)

    s/been/seen/ ?

src/executor/executor.cpp (line 415)


- 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