mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Anand Mazumdar" <mazumdar.an...@gmail.com>
Subject Re: Review Request 41283: Introduced an Executor Library based on the new executor HTTP API.
Date Fri, 22 Jan 2016 01:37:10 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message