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

Reply via email to