> On Jan. 21, 2016, 7:33 p.m., Vinod Kone wrote:
> >

Moved the code to:

`connected`: Invoke this callback after `Subscribe`/non subscribe connection 
have been established.
`disconnected`: Invoke this callback after any of `Subscribe`/non subscribe 
connection is broken.


> On Jan. 21, 2016, 7:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 170
> > <https://reviews.apache.org/r/41283/diff/8/?file=1203911#file1203911line170>
> >
> >     should be a CHECK as this is not expected.

As per our discussion, I am keeping this as is since it's in sync with 
`src/exec/exec.cpp`. Would file another patch to update this in both.


> On Jan. 21, 2016, 7:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, lines 238-259
> > <https://reviews.apache.org/r/41283/diff/8/?file=1203911#file1203911line238>
> >
> >     I think it would be a bit clearer to follow if you do something like 
> > this
> >     
> >     ```
> >     if (call.type() == Call::SUBSCRIBE) {
> >       // If we are in `CONNECTED` state, subscribe connection should exist.
> >       CHECK_SOME(subscribe);
> >       
> >       _send(call);
> >     } else {
> >       if (nonSubscribe.isSome()) {
> >         // If nonSubscribe connection already exists, use it. 
> >         _send(call);
> >       } else {
> >         // Create a new nonSubscribe connection.
> >         ...
> >       }
> >     }
> >     
> >     ```

The given code was removed. Hence, dropping the issue.


> On Jan. 21, 2016, 7:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 241
> > <https://reviews.apache.org/r/41283/diff/8/?file=1203911#file1203911line241>
> >
> >     Don't we have to check here that nonSubscribe is already set? For 
> > example, 2 back to back non-subscribe calls might result in 2 non-subscribe 
> > connections?

The given code is removed. Dropping.


> On Jan. 21, 2016, 7:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 269
> > <https://reviews.apache.org/r/41283/diff/8/?file=1203911#file1203911line269>
> >
> >     Lets comment here that the library is considered connected if we can 
> > establish the subscribe connection. Worth mentioning here that 
> > non-subscribe calls use a different `nonSubscribe` connection and that its 
> > disconnection doesn't change the `state`.

Added a comment before we invoke the `connected` callback that this is only 
invoked after we establish both `Subscribe`/Non-subscribe connections.


> On Jan. 21, 2016, 7:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 309
> > <https://reviews.apache.org/r/41283/diff/8/?file=1203911#file1203911line309>
> >
> >     How is this possible? Can you expand the comment?

Added a comment. The first time `disconnected` is invoked. We are still in 
`CONNECTED` state. Since, we backoff and retry, the connection state can again 
go from `DISCONNECTED`->`CONNECTED` with a separate `Subscribe` connection. We 
don't do anything thereafter and just return.


> On Jan. 21, 2016, 7:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, lines 376-378
> > <https://reviews.apache.org/r/41283/diff/8/?file=1203911#file1203911line376>
> >
> >     Can there be more than one oustanding connection attempts at any given 
> > time?

There cannot be. However, we need to compare the connection objects due to this 
scenario:

We disconnect from agent, start the recoveryTimeout clock.
We connect now after some time.
We disconnect again, start another recoveryTimeout clock.

We should now terminate when the second recoveryTimeout clock expires and we 
should ignore the first clock expiration. Passing the `Connection` object 
allows us to do that. This is similar to the existing logic in 
`src/exec/exec.cpp`


> On Jan. 21, 2016, 7:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 601
> > <https://reviews.apache.org/r/41283/diff/8/?file=1203911#file1203911line601>
> >
> >     can you add a comment on why you do `false` here?

Added a comment. The intention here was to process the pending `received` 
events from the agent before terminating.


> On Jan. 21, 2016, 7:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 624
> > <https://reviews.apache.org/r/41283/diff/8/?file=1203911#file1203911line624>
> >
> >     Add a comment here (or above where I commented) that the state tracks 
> > the `subscribe` connection. Also, do we really need this enum? Can't we 
> > just use `subscribe` to figure if we are connected or not?

Added a comment. Also, see my earlier comment regarding comparing connection 
object in `_recoveryTimeout`. We cannot initialize `subscribe` to `None` due to 
this. Hence, we need this enum to be the source of truth as to whether we are 
disconnected or not.


- Anand


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


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

Reply via email to