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