Re: Review Request 41283: Introduced an Executor Library based on the new executor HTTP API.

[Anand Mazumdar]

> On Jan. 25, 2016, 9:58 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 361
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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.

[Anand Mazumdar]

---
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: Introduced an Executor Library based on the new executor HTTP API.

[Vinod Kone]

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


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)


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.

[Vinod Kone]

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




src/executor/executor.cpp (line 274)


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)


formatting:

disconnected(subscribe,
 connection1.isFailed()
   ? connection1.failure()
   : "");
   
Why 'Subscribe' in quotes and not Non-subscribe below?



src/executor/executor.cpp (line 290)


no need for `else`. just make this a `if` block.



src/executor/executor.cpp (lines 291 - 294)


see formatting above.



src/executor/executor.cpp (line 299)


Maybe mention when state transitions from/to CONNECTED/DISCONNECTED?



src/executor/executor.cpp (line 308)


ditto. why quotes?



src/executor/executor.cpp (line 310)


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)


no need for quotes around Subscribe.

s/Subscribe/subscribe/



src/executor/executor.cpp (line 342)


shouldn't this be done inside `close()` ?



src/executor/executor.cpp (line 361)


dont you want to set `subscribe` and `nonSubscribe` to None()?



src/executor/executor.cpp (line 368)


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.

[Vinod Kone]

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



src/executor/executor.cpp (line 170)


should be a CHECK as this is not expected.



src/executor/executor.cpp (lines 198 - 200)


This should be a CHECK at #190, because this is not expected.



src/executor/executor.cpp (line 214)


Ditto. This should be a CHECK instead.



src/executor/executor.cpp (lines 219 - 221)


why not set these in the initializer list?



src/executor/executor.cpp (lines 238 - 259)


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)


what happens if this future is failed or discarded?



src/executor/executor.cpp (line 241)


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)


log the future's error if it is in error state.



src/executor/executor.cpp (line 269)


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)


what happens if this future is discarded?



src/executor/executor.cpp (line 300)


can we convert `_connected` into a lambda?



src/executor/executor.cpp (line 309)


How is this possible? Can you expand the comment?



src/executor/executor.cpp (lines 313 - 314)


Move this comment down to #327.



src/executor/executor.cpp (line 320)


does this need to be a function?



src/executor/executor.cpp (line 323)


can we convert `_disconnected` to a lambda?



src/executor/executor.cpp (lines 376 - 378)


Can there be more than one oustanding connection attempts at any given time?



src/executor/executor.cpp (line 596)


no need for stringify?



src/executor/executor.cpp (line 601)


can you add a comment on why you do `false` here?



src/executor/executor.cpp (line 624)


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 
>   

Re: Review Request 41283: Introduced an Executor Library based on the new executor HTTP API.

[Anand Mazumdar]

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

Re: Review Request 41283: Introduced an Executor Library based on the new executor HTTP API.

[Anand Mazumdar]

> On Jan. 14, 2016, 10:33 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 123
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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.

[Anand Mazumdar]

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

[Vinod Kone]

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



src/executor/executor.cpp (lines 20 - 21)


reorder alphabetically.



src/executor/executor.cpp (line 123)


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)


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)


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)


s/Cannot/Failed to/



src/executor/executor.cpp (line 220)


s/Cannot/Failed to/



src/executor/executor.cpp (lines 282 - 287)


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)


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)


why do we wait for `recoveryTimeout` if the agent is disconnected and we 
are not checkpointing?



src/executor/executor.cpp (line 345)


This seems like a comment for the else block.

How about

// Backoff and reconnect if checkpointing is enabled.



src/executor/executor.cpp (line 382)


s/been/seen/ ?



src/executor/executor.cpp (line 415)


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.

[Anand Mazumdar]

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

[Anand Mazumdar]

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

Re: Review Request 41283: Introduced an Executor Library based on the new executor HTTP API.

[Anand Mazumdar]

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

[Jojy Varghese]

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



src/executor/executor.cpp (line 229)


We should probably limit the max value here.



src/executor/executor.cpp (line 244)


Whats the maximum value allowed here? Might be a good idea to have it 
bounded.

Also what is the unit of time here?



src/executor/executor.cpp (line 291)


Have we considered using ```override``` keyword here?



src/executor/executor.cpp (line 370)


Have we considered using ```%``` operator to achieve the same semantics? 
Couple of reasons:

- Expresses the semantics in simple manner.
- Efficient in terms if number of instructions (1 DIV operation vs a DIV + 
MUL)


- Jojy Varghese


On Dec. 30, 2015, 1:30 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41283/
> ---
> 
> (Updated Dec. 30, 2015, 1:30 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 9762f8567e32d70b8df2d694a1fef5c692fc730e 
>   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.

[Anand Mazumdar]

> On Dec. 30, 2015, 7:25 p.m., Jojy Varghese wrote:
> > src/executor/executor.cpp, line 229
> > 
> >
> > We should probably limit the max value here.

Does not make much sense to me. We do not have any semantics in our code 
historically to define a maximum value for most values of `Flags` passed to 
Executor/Agent. Why bother setting a maximum here. What am I missing ?

Also, these flags are passed on by the agent to the executor and are not 
invalid user inputs.


> On Dec. 30, 2015, 7:25 p.m., Jojy Varghese wrote:
> > src/executor/executor.cpp, line 244
> > 
> >
> > Whats the maximum value allowed here? Might be a good idea to have it 
> > bounded.
> > 
> > Also what is the unit of time here?

Again, did not make any sense to me. How does it matter ? 

It's pretty self-explantory to see that `maxBackoff` is a `Duration` object and 
hence can take any unit of time allowed by the `Duration` object.


> On Dec. 30, 2015, 7:25 p.m., Jojy Varghese wrote:
> > src/executor/executor.cpp, line 370
> > 
> >
> > Have we considered using ```%``` operator to achieve the same 
> > semantics? Couple of reasons:
> > 
> > - Expresses the semantics in simple manner.
> > - Efficient in terms if number of instructions (1 DIV operation vs a 
> > DIV + MUL)

I am assuming you were speaking about a previous issue that I had dropped:

`backoff = ::random() % maxBackoff + 1`

^ This is also 2 instructions (`%` and `+`). Also, considering `modulo` is a 
more expensive operation then division or multiplication. This does not make 
any sense to me. What would we gain ?

Also, We have been using this approach at 99 other places and I would any day 
trade consistency in our code then gaining an iota in performance ( a gain that 
I still do not see )
https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L740
https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L1252
https://github.com/apache/mesos/blob/master/src/sched/sched.cpp#L745
... and a zillion others.


> On Dec. 30, 2015, 7:25 p.m., Jojy Varghese wrote:
> > src/executor/executor.cpp, line 291
> > 
> >
> > Have we considered using ```override``` keyword here?

Good idea. We might consider using it widely in our code in the future as we 
become more C++11.


- Anand


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


On Dec. 30, 2015, 1:30 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41283/
> ---
> 
> (Updated Dec. 30, 2015, 1:30 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 9762f8567e32d70b8df2d694a1fef5c692fc730e 
>   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.

[Jojy Varghese]

> On Dec. 29, 2015, 2:46 a.m., Jojy Varghese wrote:
> > src/executor/executor.cpp, line 221
> > 
> >
> > "else" case?
> 
> Anand Mazumdar wrote:
> Nothing needs to be done for the `else` case. These env variables 
> **might** be set by the agent when framework checkpointing is enabled. For 
> more info see how the old executor library checks it:
> https://github.com/apache/mesos/blob/master/src/exec/exec.cpp#L690
> 
> Jojy Varghese wrote:
> hrm in the else case, what would be the value of recoveryTimeout?
> 
> Anand Mazumdar wrote:
> The value would be the default value of `Duration` i.e. `0ns` i.e. you 
> would like your executor to not retry and shut down as soon as you detect a 
> disconnection with the agent (as is the case for frameworks that do not have 
> checkpointing enabled). Makes sense ?

So checkpoint with no MESOS_RECOVERY_TIMEOUT / MESOS_RETRY_MAX_BACKOFF_FACTOR 
is same as no checkpoint?


- Jojy


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


On Dec. 29, 2015, 3:11 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41283/
> ---
> 
> (Updated Dec. 29, 2015, 3:11 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 9762f8567e32d70b8df2d694a1fef5c692fc730e 
>   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.

[Jojy Varghese]

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



src/executor/executor.cpp (line 363)


Why not ``` backoff = ::random() % maxBackoff + 1 ```?

In the current form, wouldnt backoff could always be 0(when env variable 
for max backoff is not present)?


- Jojy Varghese


On Dec. 29, 2015, 3:11 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41283/
> ---
> 
> (Updated Dec. 29, 2015, 3:11 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 9762f8567e32d70b8df2d694a1fef5c692fc730e 
>   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.

[Jojy Varghese]

> On Dec. 29, 2015, 2:46 a.m., Jojy Varghese wrote:
> > src/executor/executor.cpp, line 221
> > 
> >
> > "else" case?
> 
> Anand Mazumdar wrote:
> Nothing needs to be done for the `else` case. These env variables 
> **might** be set by the agent when framework checkpointing is enabled. For 
> more info see how the old executor library checks it:
> https://github.com/apache/mesos/blob/master/src/exec/exec.cpp#L690
> 
> Jojy Varghese wrote:
> hrm in the else case, what would be the value of recoveryTimeout?
> 
> Anand Mazumdar wrote:
> The value would be the default value of `Duration` i.e. `0ns` i.e. you 
> would like your executor to not retry and shut down as soon as you detect a 
> disconnection with the agent (as is the case for frameworks that do not have 
> checkpointing enabled). Makes sense ?
> 
> Jojy Varghese wrote:
> So checkpoint with no MESOS_RECOVERY_TIMEOUT / 
> MESOS_RETRY_MAX_BACKOFF_FACTOR is same as no checkpoint?

I think if framework intends to be “checkpointed”, then we shud have a warning 
when “recoverytimeout” is not present because the only thing checkpoint env 
variable provides is recoveryTimeout and maxBackOff.


- Jojy


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


On Dec. 29, 2015, 3:11 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41283/
> ---
> 
> (Updated Dec. 29, 2015, 3:11 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 9762f8567e32d70b8df2d694a1fef5c692fc730e 
>   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.

[Anand Mazumdar]

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

(Updated Dec. 29, 2015, 3:11 a.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
---

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 9762f8567e32d70b8df2d694a1fef5c692fc730e 
  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.

[Anand Mazumdar]

> On Dec. 29, 2015, 2:46 a.m., Jojy Varghese wrote:
> > src/executor/executor.cpp, line 221
> > 
> >
> > "else" case?

Nothing needs to be done for the `else` case. These env variables **might** be 
set by the agent when framework checkpointing is enabled. For more info see how 
the old executor library checks it:
https://github.com/apache/mesos/blob/master/src/exec/exec.cpp#L690


> On Dec. 29, 2015, 2:46 a.m., Jojy Varghese wrote:
> > src/executor/executor.cpp, line 232
> > 
> >
> > "else" case?

Ditto as above


> On Dec. 29, 2015, 2:46 a.m., Jojy Varghese wrote:
> > src/executor/executor.cpp, line 266
> > 
> >
> > Why not use universal initialization ({})?

`Request` struct has a lot of fields all of which are not used by us. Hence, 
univeral initialization is not possible. 
https://github.com/apache/mesos/blob/master/3rdparty/libprocess/include/process/http.hpp#L224


- Anand


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


On Dec. 29, 2015, 12:11 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41283/
> ---
> 
> (Updated Dec. 29, 2015, 12:11 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 9762f8567e32d70b8df2d694a1fef5c692fc730e 
>   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.

[Jojy Varghese]

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



src/executor/executor.cpp (line 54)


Do you really need the entire slave namespace or something narrower like 
``` mesos::internal::slave::validation::executor```



src/executor/executor.cpp (line 118)


const?



src/executor/executor.cpp (line 183)


Why not UPID ```upid(value.get());```



src/executor/executor.cpp (line 221)


"else" case?



src/executor/executor.cpp (line 232)


"else" case?



src/executor/executor.cpp (line 255)


See comment about namespace scope.



src/executor/executor.cpp (line 256)


why blank line?



src/executor/executor.cpp (line 264)


Why declare response far from its use? Its preferred that the declarations 
be closer to their use.



src/executor/executor.cpp (line 266)


Why not use universal initialization ({})?


- Jojy Varghese


On Dec. 29, 2015, 12:11 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41283/
> ---
> 
> (Updated Dec. 29, 2015, 12:11 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 9762f8567e32d70b8df2d694a1fef5c692fc730e 
>   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.

[Jojy Varghese]

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



src/executor/executor.cpp (line 354)


What is the purpose of mutex? It looks like we do the callbacks 
asynchronously. Which means, we have the lock only till its dispatched. We dont 
know when its executed. So wouldnt that mean two callbacks can be executed 
asyncronously at the same time?


- Jojy Varghese


On Dec. 29, 2015, 3:11 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41283/
> ---
> 
> (Updated Dec. 29, 2015, 3:11 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 9762f8567e32d70b8df2d694a1fef5c692fc730e 
>   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.

[Anand Mazumdar]

> On Dec. 29, 2015, 2:46 a.m., Jojy Varghese wrote:
> > src/executor/executor.cpp, line 221
> > 
> >
> > "else" case?
> 
> Anand Mazumdar wrote:
> Nothing needs to be done for the `else` case. These env variables 
> **might** be set by the agent when framework checkpointing is enabled. For 
> more info see how the old executor library checks it:
> https://github.com/apache/mesos/blob/master/src/exec/exec.cpp#L690
> 
> Jojy Varghese wrote:
> hrm in the else case, what would be the value of recoveryTimeout?

The value would be the default value of `Duration` i.e. `0ns` i.e. you would 
like your executor to not retry and shut down as soon as you detect a 
disconnection with the agent (as is the case for frameworks that do not have 
checkpointing enabled). Makes sense ?


- Anand


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


On Dec. 29, 2015, 3:11 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41283/
> ---
> 
> (Updated Dec. 29, 2015, 3:11 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 9762f8567e32d70b8df2d694a1fef5c692fc730e 
>   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.

[Anand Mazumdar]

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

(Updated Dec. 29, 2015, 12:11 a.m.)


Review request for mesos, Ben Mahler and Vinod Kone.


Changes
---

Added logic for retries with linear backoff once disconnected for the agent.


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 9762f8567e32d70b8df2d694a1fef5c692fc730e 
  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.

[Anand Mazumdar]

> On Dec. 29, 2015, 6:09 a.m., Jojy Varghese wrote:
> > src/executor/executor.cpp, line 354
> > 
> >
> > What is the purpose of mutex? It looks like we do the callbacks 
> > asynchronously. Which means, we have the lock only till its dispatched. We 
> > dont know when its executed. So wouldnt that mean two callbacks can be 
> > executed asyncronously at the same time?

Can you elaborate a bit more ? If you have a look at the implementation of 
`async` the `Future` is only fullfilled once the callback has been successfully 
invoked not **before**. Hence, the mutex is only unlocked after the callback is 
**completed**. 

https://github.com/apache/mesos/blob/master/3rdparty/libprocess/include/process/async.hpp#L93

The scheduler library also follows a similar pattern for invoking the callbacks 
: https://github.com/apache/mesos/blob/master/src/scheduler/scheduler.cpp

Hope this info helps.


- Anand


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


On Dec. 29, 2015, 3:11 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41283/
> ---
> 
> (Updated Dec. 29, 2015, 3:11 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 9762f8567e32d70b8df2d694a1fef5c692fc730e 
>   src/executor/executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41283/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>