You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Anand Mazumdar <ma...@gmail.com> on 2016/02/17 20:22:52 UTC
Review Request 43662: Added support for pipelining calls to the
scheduler library.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43662/
-----------------------------------------------------------
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
Re: Review Request 43662: Added support for pipelining calls to the
scheduler library.
Posted by Anand Mazumdar <ma...@gmail.com>.
> 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
>
>
Re: Review Request 43662: Added support for pipelining calls to the
scheduler library.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43662/#review120426
-----------------------------------------------------------
src/scheduler/scheduler.cpp (lines 117 - 119)
<https://reviews.apache.org/r/43662/#comment181864>
This comment is old. Can you update?
s/receiving messages (eventually events/HTTP messages/
s/sending messages (via calls)/HTTP messages/
src/scheduler/scheduler.cpp (line 207)
<https://reviews.apache.org/r/43662/#comment181865>
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.
src/scheduler/scheduler.cpp (lines 237 - 242)
<https://reviews.apache.org/r/43662/#comment181866>
Shouldn't we do the same for executor library?
More importnatly, what if a previous subscribe is in progress but `subscribed` is not set?
src/scheduler/scheduler.cpp (line 264)
<https://reviews.apache.org/r/43662/#comment181872>
CHECK(state == DISCONNECTED) ?
src/scheduler/scheduler.cpp (lines 278 - 280)
<https://reviews.apache.org/r/43662/#comment181873>
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....?
src/scheduler/scheduler.cpp (line 297)
<https://reviews.apache.org/r/43662/#comment181876>
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?
src/scheduler/scheduler.cpp (line 370)
<https://reviews.apache.org/r/43662/#comment181886>
is it possible for this callback to be invoked on the scheduler after they get a connected callback from this new connection attempt?
src/scheduler/scheduler.cpp (line 400)
<https://reviews.apache.org/r/43662/#comment181891>
What happens if a previous connection attempt is in progress?
src/scheduler/scheduler.cpp (line 457)
<https://reviews.apache.org/r/43662/#comment181887>
For executor library, we did a close() here of the previous reader. Why was it required there and not here?
- Vinod Kone
On Feb. 22, 2016, 8:19 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43662/
> -----------------------------------------------------------
>
> (Updated Feb. 22, 2016, 8:19 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
>
>
Re: Review Request 43662: Added support for pipelining calls to the
scheduler library.
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43662/
-----------------------------------------------------------
(Updated Feb. 27, 2016, 10:30 a.m.)
Review request for mesos and Vinod Kone.
Changes
-------
Used UNREACHABLE to make gcc happy.
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 (updated)
-----
src/scheduler/scheduler.cpp 99a7d0dfff7b0c61decc9ff6d9e6d46ef13a7e75
Diff: https://reviews.apache.org/r/43662/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar
Re: Review Request 43662: Added support for pipelining calls to the
scheduler library.
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43662/
-----------------------------------------------------------
(Updated Feb. 27, 2016, 4:43 a.m.)
Review request for mesos and Vinod Kone.
Changes
-------
Review comments
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 (updated)
-----
src/scheduler/scheduler.cpp 99a7d0dfff7b0c61decc9ff6d9e6d46ef13a7e75
Diff: https://reviews.apache.org/r/43662/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar
Re: Review Request 43662: Added support for pipelining calls to the
scheduler library.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43662/#review121046
-----------------------------------------------------------
Fix it, then Ship it!
src/scheduler/scheduler.cpp (line 276)
<https://reviews.apache.org/r/43662/#comment182632>
Can you expand more here. Need to say that we make a copy here because 'master' and 'connectionId' values might change by the time the 2nd `http::connect()` or `connected()` gets called.
src/scheduler/scheduler.cpp (line 282)
<https://reviews.apache.org/r/43662/#comment182631>
s/master.get()/master_/
src/scheduler/scheduler.cpp (line 302)
<https://reviews.apache.org/r/43662/#comment182633>
"Ignoring stale connection attempt"
This can happen even without master failover.
src/scheduler/scheduler.cpp (line 358)
<https://reviews.apache.org/r/43662/#comment182635>
ditto. VLOG(1) ?
src/scheduler/scheduler.cpp (line 392)
<https://reviews.apache.org/r/43662/#comment182636>
I think this will read better as
if (state == CONNECTED || state == SUBSCRIBING || state == SUBSCRIBED)
- Vinod Kone
On Feb. 27, 2016, 1:22 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43662/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2016, 1:22 a.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
>
>
Re: Review Request 43662: Added support for pipelining calls to the
scheduler library.
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43662/
-----------------------------------------------------------
(Updated Feb. 27, 2016, 1:22 a.m.)
Review request for mesos and Vinod Kone.
Changes
-------
Review comments
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 (updated)
-----
src/scheduler/scheduler.cpp 99a7d0dfff7b0c61decc9ff6d9e6d46ef13a7e75
Diff: https://reviews.apache.org/r/43662/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar
Re: Review Request 43662: Added support for pipelining calls to the
scheduler library.
Posted by Anand Mazumdar <ma...@gmail.com>.
> On Feb. 26, 2016, 8:29 p.m., Vinod Kone wrote:
> > src/scheduler/scheduler.cpp, line 394
> > <https://reviews.apache.org/r/43662/diff/3/?file=1271056#file1271056line394>
> >
> > what if we are in CONNECTING state?
Good catch. I had missed that. Added a explicit check for CONNECTING too.
> On Feb. 26, 2016, 8:29 p.m., Vinod Kone wrote:
> > src/scheduler/scheduler.cpp, line 585
> > <https://reviews.apache.org/r/43662/diff/3/?file=1271056#file1271056line585>
> >
> > what about other states?
Added an explicit check for SUBSCRIBED.
> On Feb. 26, 2016, 8:29 p.m., Vinod Kone wrote:
> > src/scheduler/scheduler.cpp, line 549
> > <https://reviews.apache.org/r/43662/diff/3/?file=1271056#file1271056line549>
> >
> > Do we need 'subscribed' as a member variable now? Can we not detect this via state and/or connectionID?
As per our discussion, let me do this change in a separate patch both for scheduler/executor.
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43662/#review120947
-----------------------------------------------------------
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
>
>
Re: Review Request 43662: Added support for pipelining calls to the
scheduler library.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43662/#review120947
-----------------------------------------------------------
src/scheduler/scheduler.cpp (lines 203 - 206)
<https://reviews.apache.org/r/43662/#comment182487>
kill this and do all the drops after #213.
src/scheduler/scheduler.cpp (line 214)
<https://reviews.apache.org/r/43662/#comment182489>
//.....
if (call.type() == SUBSCRIBE && state != CONNECTED) {
drop();
return;
}
//.....
if (call.type() != SUBSCRIBE && state != SUBSCRIBED) {
drop();
return;
}
src/scheduler/scheduler.cpp (lines 232 - 239)
<https://reviews.apache.org/r/43662/#comment182490>
Kill this.
src/scheduler/scheduler.cpp (lines 233 - 239)
<https://reviews.apache.org/r/43662/#comment182488>
It's better to consolidate all drops before #215.
src/scheduler/scheduler.cpp (line 246)
<https://reviews.apache.org/r/43662/#comment182486>
shouldn't we drop calls if we are not in SUBSCRIBED state? see above.
src/scheduler/scheduler.cpp (line 249)
<https://reviews.apache.org/r/43662/#comment182491>
CHECK_SOME(connectionId); ?
also, why is this method taking an Option<ConnectionID> instead of ConnectionID?
src/scheduler/scheduler.cpp (line 276)
<https://reviews.apache.org/r/43662/#comment182492>
why is this taking an option?
src/scheduler/scheduler.cpp (line 290)
<https://reviews.apache.org/r/43662/#comment182493>
Add a VLOG(1)?
src/scheduler/scheduler.cpp (line 378)
<https://reviews.apache.org/r/43662/#comment182494>
what if we are in CONNECTING state?
src/scheduler/scheduler.cpp (line 387)
<https://reviews.apache.org/r/43662/#comment182495>
s/old//
src/scheduler/scheduler.cpp (line 457)
<https://reviews.apache.org/r/43662/#comment182496>
what about other states? can we be in CONNECTING here for example?
src/scheduler/scheduler.cpp (line 498)
<https://reviews.apache.org/r/43662/#comment182497>
// We reset the state to CONNECTED if...
src/scheduler/scheduler.cpp (line 533)
<https://reviews.apache.org/r/43662/#comment182500>
Do we need 'subscribed' as a member variable now? Can we not detect this via state and/or connectionID?
src/scheduler/scheduler.cpp (line 546)
<https://reviews.apache.org/r/43662/#comment182498>
Can you add a comment here on when this can happen?
src/scheduler/scheduler.cpp (line 567)
<https://reviews.apache.org/r/43662/#comment182499>
what about other states?
src/scheduler/scheduler.cpp (line 609)
<https://reviews.apache.org/r/43662/#comment182482>
s/connection/connections/
src/scheduler/scheduler.cpp (line 610)
<https://reviews.apache.org/r/43662/#comment182483>
s/connection/connections/
- Vinod Kone
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
>
>
Re: Review Request 43662: Added support for pipelining calls to the
scheduler library.
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
Review comments from Vinod
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 (updated)
-----
src/scheduler/scheduler.cpp 99a7d0dfff7b0c61decc9ff6d9e6d46ef13a7e75
Diff: https://reviews.apache.org/r/43662/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar
Re: Review Request 43662: Added support for pipelining calls to the
scheduler library.
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43662/
-----------------------------------------------------------
(Updated Feb. 22, 2016, 8:19 p.m.)
Review request for mesos and Vinod Kone.
Changes
-------
Rebased
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 (updated)
-----
src/scheduler/scheduler.cpp 99a7d0dfff7b0c61decc9ff6d9e6d46ef13a7e75
Diff: https://reviews.apache.org/r/43662/diff/
Testing
-------
make check
Thanks,
Anand Mazumdar