----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41283/#review116179 -----------------------------------------------------------
src/executor/executor.cpp (line 274) <https://reviews.apache.org/r/41283/#comment177180> Can you add a comment here that you are making 2 persistent connections here, one for subscribe call (and its streaming response) and another for nonSubscribe calls. src/executor/executor.cpp (lines 286 - 288) <https://reviews.apache.org/r/41283/#comment177181> formatting: disconnected(subscribe, connection1.isFailed() ? connection1.failure() : "...."); Why 'Subscribe' in quotes and not Non-subscribe below? src/executor/executor.cpp (line 290) <https://reviews.apache.org/r/41283/#comment177183> no need for `else`. just make this a `if` block. src/executor/executor.cpp (lines 291 - 294) <https://reviews.apache.org/r/41283/#comment177182> see formatting above. src/executor/executor.cpp (line 299) <https://reviews.apache.org/r/41283/#comment177184> Maybe mention when state transitions from/to CONNECTED/DISCONNECTED? src/executor/executor.cpp (line 308) <https://reviews.apache.org/r/41283/#comment177185> ditto. why quotes? src/executor/executor.cpp (line 310) <https://reviews.apache.org/r/41283/#comment177186> s/Subscribe/subscribe/ Also have this comment up at #291? More importantly, didn't we decide to close both connections if any of them fails/disconnects? I don't see that code in this patch? src/executor/executor.cpp (line 318) <https://reviews.apache.org/r/41283/#comment177187> no need for quotes around Subscribe. s/Subscribe/subscribe/ src/executor/executor.cpp (line 342) <https://reviews.apache.org/r/41283/#comment177191> shouldn't this be done inside `close()` ? src/executor/executor.cpp (line 361) <https://reviews.apache.org/r/41283/#comment177193> dont you want to set `subscribe` and `nonSubscribe` to None()? src/executor/executor.cpp (line 368) <https://reviews.apache.org/r/41283/#comment177190> Why is this outside the `if (state == CONNECTED)` block? I'm guessing you don't want to fire multiple delays one for subscribe disconnection and nonSubscribe disconnection for example? If yes, you might just want to do the below at the top of this method. ``` if (state == DISCONNECTED) { return; } ``` and merge this if block with the one at #352. - Vinod Kone On Jan. 22, 2016, 1:36 a.m., Anand Mazumdar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41283/ > ----------------------------------------------------------- > > (Updated Jan. 22, 2016, 1:36 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 for the `Subscribe` call. > `disconnected` -> When the `Subscribe` 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 d23e35001078a86775bd9b76baa207ecb9dab7e1 > src/executor/executor.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/41283/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Anand Mazumdar > >
