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 Wed, 20 Jan 2016 22:12:14 GMT


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


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