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 43662: Added support for pipelining calls to the scheduler library.
Date Thu, 25 Feb 2016 16:28:47 GMT


> On Feb. 24, 2016, 3:03 a.m., Vinod Kone wrote:
> > src/scheduler/scheduler.cpp, lines 239-244
> > <https://reviews.apache.org/r/43662/diff/2/?file=1264787#file1264787line239>
> >
> >     Shouldn't we do the same for executor library?
> >     
> >     More importnatly, what if a previous subscribe is in progress but `subscribed`
is not set?

As per our discussion introduced two more states, SUBSCRIBING/SUBSCRIBED to take care of this.


In the initial version, we would have invoked the `disconnected` callback if we noticed the
race which would not have been ideal.


> On Feb. 24, 2016, 3:03 a.m., Vinod Kone wrote:
> > src/scheduler/scheduler.cpp, lines 290-292
> > <https://reviews.apache.org/r/43662/diff/2/?file=1264787#file1264787line290>
> >
> >     s/i.e./e.g.,/ ?
> >     
> >     s/we failed over to a new master/master failed over/
> >     
> >     Also, not sure what you wanted to say in this comment? Are you saying it is
OK even if a new master is enqueued because....?

Removed this block of code.


> On Feb. 24, 2016, 3:03 a.m., Vinod Kone wrote:
> > src/scheduler/scheduler.cpp, line 309
> > <https://reviews.apache.org/r/43662/diff/2/?file=1264787#file1264787line309>
> >
> >     This is the current master as detected by the detector. What is the guarantee
that the connection was made with this master and not an old master? IOW, should this method
take `master` as an argument?
> >     
> >     Can we guarantee that the master hasn't changed because a new master detected
causes these futures to be discarded synchronously? Looking at detected() that doesn't seem
to be the case.
> >     
> >     Do we want to enforce the invariant that there is ever only one connection attempt
in progress?

As per our offline discussion, we would be assigning a new UUID for the connection instance
before we initiate the connection and not after the connection has been established.


> On Feb. 24, 2016, 3:03 a.m., Vinod Kone wrote:
> > src/scheduler/scheduler.cpp, line 387
> > <https://reviews.apache.org/r/43662/diff/2/?file=1264787#file1264787line387>
> >
> >     is it possible for this callback to be invoked on the scheduler after they get
a connected callback from this new connection attempt?

Not quite. As per our discussion earlier, the mutex gurrantees that the calls are serialized
in order.


> On Feb. 24, 2016, 3:03 a.m., Vinod Kone wrote:
> > src/scheduler/scheduler.cpp, line 474
> > <https://reviews.apache.org/r/43662/diff/2/?file=1264787#file1264787line474>
> >
> >     For executor library, we did a close() here of the previous reader. Why was
it required there and not here?

Good catch. Since with pipelining, its now not possible to send two `Subscribe` calls on the
same connection. We would need a similar change for the Executor library to not invoke `close()`.


> On Feb. 24, 2016, 3:03 a.m., Vinod Kone wrote:
> > src/scheduler/scheduler.cpp, line 209
> > <https://reviews.apache.org/r/43662/diff/2/?file=1264787#file1264787line209>
> >
> >     I think for both scheduler::Mesos and executor::Mesos we need to add more comments
around send() semantics. 
> >     
> >     For example, we should mention that clients should send() only after connected()
callback has been called.

As per our discussion and also my comment on r43661, I would add the comments in a separate
patch for both the scheduler/executor as they both have the same semantics.


- Anand


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


On Feb. 25, 2016, 4:27 p.m., Anand Mazumdar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43662/
> -----------------------------------------------------------
> 
> (Updated Feb. 25, 2016, 4:27 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3570
>     https://issues.apache.org/jira/browse/MESOS-3570
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Previously, the scheduler library used to chain calls on previous call responses. This
was inherently slow. This change adds support for pipelining all calls to the master on a
single connection via the `http::Connection` abstraction in libprocess.
> 
> This change also adds support for handling various error scenarios when we notice a disconnection
instead of just relying on the master detector for invoking the `disconnected` callback.
> 
> 
> Diffs
> -----
> 
>   src/scheduler/scheduler.cpp 99a7d0dfff7b0c61decc9ff6d9e6d46ef13a7e75 
> 
> Diff: https://reviews.apache.org/r/43662/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>


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