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