You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Anand Mazumdar <ma...@gmail.com> on 2016/01/06 02:29:53 UTC
Re: Review Request 41283: Introduced an Executor Library based on the
new executor HTTP API.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41283/
-----------------------------------------------------------
(Updated Jan. 6, 2016, 1:29 a.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Changes
-------
Modified env variable name.
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 (updated)
-----
src/Makefile.am 8af0115caa67ac8f3193d8f0d0f1a4ae739af275
src/executor/executor.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/41283/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar
Re: Review Request 41283: Introduced an Executor Library based on the
new executor HTTP API.
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41283/
-----------------------------------------------------------
(Updated Jan. 26, 2016, 10:29 p.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Changes
-------
Review comments from Vinod
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 (updated)
-----
src/Makefile.am d23e35001078a86775bd9b76baa207ecb9dab7e1
src/executor/executor.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/41283/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar
Re: Review Request 41283: Introduced an Executor Library based on the
new executor HTTP API.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41283/#review116455
-----------------------------------------------------------
Fix it, then Ship it!
Looking good. Just some minor things.
src/executor/executor.cpp (lines 353 - 355)
<https://reviews.apache.org/r/41283/#comment177475>
Move this to #364.
How about:
// NOTE: We will be here if either subscribe or non-subscribe connection is disconnected. We explicitly
// disconnect both the connections here for simplicity.
src/executor/executor.cpp (line 430)
<https://reviews.apache.org/r/41283/#comment177478>
just do
```
// Ignore if this timeout is for a stale connection.
if (connections != _connections) {
return;
}
```
to be consistent with what you did in `disconnected(()`
- Vinod Kone
On Jan. 26, 2016, 9:19 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41283/
> -----------------------------------------------------------
>
> (Updated Jan. 26, 2016, 9:19 p.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
>
>
Re: Review Request 41283: Introduced an Executor Library based on the
new executor HTTP API.
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41283/
-----------------------------------------------------------
(Updated Jan. 26, 2016, 9:19 p.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Changes
-------
Updated Summary/Description back again.
Summary (updated)
-----------------
Introduced an Executor Library based on the new executor HTTP API.
Bugs: MESOS-3550
https://issues.apache.org/jira/browse/MESOS-3550
Repository: mesos
Description (updated)
-------
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
Re: Review Request 41283: V1 Executor Library over HTTP
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41283/
-----------------------------------------------------------
(Updated Jan. 26, 2016, 8:33 p.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Changes
-------
Review comments from Vinod
Summary (updated)
-----------------
V1 Executor Library over HTTP
Bugs: MESOS-3550
https://issues.apache.org/jira/browse/MESOS-3550
Repository: mesos
Description (updated)
-------
V1 Executor Library over HTTP
Diffs (updated)
-----
src/Makefile.am d23e35001078a86775bd9b76baa207ecb9dab7e1
src/executor/executor.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/41283/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar
Re: Review Request 41283: Introduced an Executor Library based on the
new executor HTTP API.
Posted by Anand Mazumdar <ma...@gmail.com>.
> On Jan. 25, 2016, 9:58 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 361
> > <https://reviews.apache.org/r/41283/diff/9/?file=1205244#file1205244line361>
> >
> > dont you want to set `subscribe` and `nonSubscribe` to None()?
As per our discussion, we don't set them to `None()` since we need them later when `_recoveryTimeout` is invoked.
> On Jan. 25, 2016, 9:58 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 368
> > <https://reviews.apache.org/r/41283/diff/9/?file=1205244#file1205244line368>
> >
> > 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.
Fixed. Previously, we used to fire 2 retries for both the subscribe/non-subscribe connections. Thanks for noticing it. As per our discussion, now we retry inside the `backoff` function instead of `disconnected` only once per disconnection.
> On Jan. 25, 2016, 9:58 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 299
> > <https://reviews.apache.org/r/41283/diff/9/?file=1205244#file1205244line299>
> >
> > Maybe mention when state transitions from/to CONNECTED/DISCONNECTED?
We mention them when we define the `enum`. Let me know if that is not enough.
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41283/#review116179
-----------------------------------------------------------
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
>
>
Re: Review Request 41283: Introduced an Executor Library based on the
new executor HTTP API.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 41283: Introduced an Executor Library based on the
new executor HTTP API.
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
Review comments from Vinod
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 (updated)
-----
src/Makefile.am d23e35001078a86775bd9b76baa207ecb9dab7e1
src/executor/executor.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/41283/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar
Re: Review Request 41283: Introduced an Executor Library based on the
new executor HTTP API.
Posted by Anand Mazumdar <ma...@gmail.com>.
> 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
>
>
Re: Review Request 41283: Introduced an Executor Library based on the
new executor HTTP API.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41283/#review115502
-----------------------------------------------------------
src/executor/executor.cpp (line 170)
<https://reviews.apache.org/r/41283/#comment176479>
should be a CHECK as this is not expected.
src/executor/executor.cpp (lines 198 - 200)
<https://reviews.apache.org/r/41283/#comment176477>
This should be a CHECK at #190, because this is not expected.
src/executor/executor.cpp (line 214)
<https://reviews.apache.org/r/41283/#comment176478>
Ditto. This should be a CHECK instead.
src/executor/executor.cpp (lines 219 - 221)
<https://reviews.apache.org/r/41283/#comment176480>
why not set these in the initializer list?
src/executor/executor.cpp (lines 238 - 259)
<https://reviews.apache.org/r/41283/#comment176487>
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.
...
}
}
```
src/executor/executor.cpp (line 239)
<https://reviews.apache.org/r/41283/#comment176492>
what happens if this future is failed or discarded?
src/executor/executor.cpp (line 241)
<https://reviews.apache.org/r/41283/#comment176493>
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?
src/executor/executor.cpp (line 247)
<https://reviews.apache.org/r/41283/#comment176488>
log the future's error if it is in error state.
src/executor/executor.cpp (line 269)
<https://reviews.apache.org/r/41283/#comment176491>
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`.
src/executor/executor.cpp (line 274)
<https://reviews.apache.org/r/41283/#comment176490>
what happens if this future is discarded?
src/executor/executor.cpp (line 300)
<https://reviews.apache.org/r/41283/#comment176673>
can we convert `_connected` into a lambda?
src/executor/executor.cpp (line 309)
<https://reviews.apache.org/r/41283/#comment176675>
How is this possible? Can you expand the comment?
src/executor/executor.cpp (lines 313 - 314)
<https://reviews.apache.org/r/41283/#comment176676>
Move this comment down to #327.
src/executor/executor.cpp (line 320)
<https://reviews.apache.org/r/41283/#comment176678>
does this need to be a function?
src/executor/executor.cpp (line 323)
<https://reviews.apache.org/r/41283/#comment176674>
can we convert `_disconnected` to a lambda?
src/executor/executor.cpp (lines 376 - 378)
<https://reviews.apache.org/r/41283/#comment176680>
Can there be more than one oustanding connection attempts at any given time?
src/executor/executor.cpp (line 596)
<https://reviews.apache.org/r/41283/#comment176687>
no need for stringify?
src/executor/executor.cpp (line 601)
<https://reviews.apache.org/r/41283/#comment176688>
can you add a comment on why you do `false` here?
src/executor/executor.cpp (line 624)
<https://reviews.apache.org/r/41283/#comment176689>
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?
- Vinod Kone
On Jan. 20, 2016, 10:11 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41283/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2016, 10:11 p.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
>
>
Re: Review Request 41283: Introduced an Executor Library based on the
new executor HTTP API.
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41283/
-----------------------------------------------------------
(Updated Jan. 20, 2016, 10:11 p.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Changes
-------
Review comments
Bugs: MESOS-3550
https://issues.apache.org/jira/browse/MESOS-3550
Repository: mesos
Description (updated)
-------
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 (updated)
-----
src/Makefile.am d23e35001078a86775bd9b76baa207ecb9dab7e1
src/executor/executor.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/41283/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar
Re: Review Request 41283: Introduced an Executor Library based on the
new executor HTTP API.
Posted by Anand Mazumdar <ma...@gmail.com>.
> On Jan. 14, 2016, 10:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 123
> > <https://reviews.apache.org/r/41283/diff/7/?file=1193992#file1193992line123>
> >
> > The name of this struct is weird considering it is encapsulating two connections.
> >
> > s/HttpConnection/connections/ ?
> >
> > Also, do we really need a struct?
Dropped this `struct` and instead used two members for the `Connection` object called `subscribe`/`nonSubscribe`.
> On Jan. 14, 2016, 10:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, lines 179-183
> > <https://reviews.apache.org/r/41283/diff/7/?file=1193992#file1193992line179>
> >
> > 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?
Yep, there is. But we need `MESOS_SLAVE_PID` for testing since we don't have delegates set for `slave(X)` -> root i.e. `/api/v1/executor`. As per our discussion, I would file a separate JIRA for this since the scheduler also uses the `PID` from the master currently.
> On Jan. 14, 2016, 10:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, lines 282-287
> > <https://reviews.apache.org/r/41283/diff/7/?file=1193992#file1193992line282>
> >
> > 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.
Fixed. Now we create the `Subscribe` connection initially and invoke the `connected` callback thereafter. We only create the second connection on a need basis and don't invoke the `disconnected` callback if it breaks. The `disconnected` callback is only invoked for the `Subscribe` connection.
> On Jan. 14, 2016, 10:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 334
> > <https://reviews.apache.org/r/41283/diff/7/?file=1193992#file1193992line334>
> >
> > why do we wait for `recoveryTimeout` if the agent is disconnected and we are not checkpointing?
I was initially relying on `recoveryTimeout` to be `0` initially. Changed it to be an `Option`.
> On Jan. 14, 2016, 10:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 382
> > <https://reviews.apache.org/r/41283/diff/7/?file=1193992#file1193992line382>
> >
> > s/been/seen/ ?
This wasn't clear to me. This comment is similar to the already existing one in `src/exec/exec.cpp`. Feel free to reopen.
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41283/#review114253
-----------------------------------------------------------
On Jan. 20, 2016, 10:11 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41283/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2016, 10:11 p.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
>
>
Re: Review Request 41283: Introduced an Executor Library based on the
new executor HTTP API.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 41283: Introduced an Executor Library based on the
new executor HTTP API.
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
Rebased
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 (updated)
-----
src/Makefile.am 8cbfb1ba5fa49f2d3cc26ea325838a1c68a79660
src/executor/executor.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/41283/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar
Re: Review Request 41283: Introduced an Executor Library based on the
new executor HTTP API.
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41283/
-----------------------------------------------------------
(Updated Jan. 11, 2016, 8:13 p.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Changes
-------
Removed some environment variables that were used only for sanity/validation purposes by the library. Leaving this for the `Executors` to implement this logic.
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 (updated)
-----
src/Makefile.am 8cbfb1ba5fa49f2d3cc26ea325838a1c68a79660
src/executor/executor.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/41283/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar