You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2012/09/26 20:01:47 UTC
Review Request: Changed the ZooKeeper master detector to "time out" a
session for instances that are being used to contend for leadership (and
send a NoMasterDetectedMessage and expire the session after a reconnect
does occur).
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7295/
-----------------------------------------------------------
Review request for mesos, John Sirois, Vinod Kone, and Ben Mahler.
Description
-------
See summary (7 of 7).
Diffs
-----
src/detector/detector.cpp 0246846
third_party/libprocess/include/process/delay.hpp cb2fa9a
third_party/libprocess/include/process/timer.hpp 333a806
third_party/libprocess/src/process.cpp fde7154
Diff: https://reviews.apache.org/r/7295/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request: Changed the ZooKeeper master detector to "time out" a
session for instances that are being used to contend for leadership (and
send a NoMasterDetectedMessage and expire the session after a reconnect
does occur).
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Sept. 26, 2012, 11:54 p.m., John Sirois wrote:
> > src/detector/detector.cpp, line 382
> > <https://reviews.apache.org/r/7295/diff/1/?file=160216#file160216line382>
> >
> > Any reason not to call expired() instead?
Because that's an "event" from ZooKeeper and I wanted to keep the semantics of that assuming it would only be called in the that case.
> On Sept. 26, 2012, 11:54 p.m., John Sirois wrote:
> > src/detector/detector.cpp, line 396
> > <https://reviews.apache.org/r/7295/diff/1/?file=160216#file160216line396>
> >
> > prematurely
Thank you.
> On Sept. 26, 2012, 11:54 p.m., John Sirois wrote:
> > src/detector/detector.cpp, line 439
> > <https://reviews.apache.org/r/7295/diff/1/?file=160216#file160216line439>
> >
> > expired = true;
Well, 'expire = false', but yeah. ;)
> On Sept. 26, 2012, 11:54 p.m., John Sirois wrote:
> > src/detector/detector.cpp, line 467
> > <https://reviews.apache.org/r/7295/diff/1/?file=160216#file160216line467>
> >
> > Why not directly expire() here instead of marking state and then waiting for connected() before finally expire()ing ?
It's possible that there is already a message in our queue to invoke one of the ZooKeeper callbacks (e.g., connected, expired, etc), so we need to delete and recreate another ZooKeeper instance from within those callbacks (to guarantee that the next callbacks we get are associated with the current ZooKeeper instance we have).
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7295/#review11957
-----------------------------------------------------------
On Sept. 26, 2012, 6:01 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7295/
> -----------------------------------------------------------
>
> (Updated Sept. 26, 2012, 6:01 p.m.)
>
>
> Review request for mesos, John Sirois, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary (7 of 7).
>
>
> Diffs
> -----
>
> src/detector/detector.cpp 0246846
> third_party/libprocess/include/process/delay.hpp cb2fa9a
> third_party/libprocess/include/process/timer.hpp 333a806
> third_party/libprocess/src/process.cpp fde7154
>
> Diff: https://reviews.apache.org/r/7295/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Changed the ZooKeeper master detector to "time out" a
session for instances that are being used to contend for leadership (and
send a NoMasterDetectedMessage and expire the session after a reconnect
does occur).
Posted by John Sirois <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7295/#review11957
-----------------------------------------------------------
src/detector/detector.cpp
<https://reviews.apache.org/r/7295/#comment25489>
Took a the trip down memory lane - Option = handles the memory leak
src/detector/detector.cpp
<https://reviews.apache.org/r/7295/#comment25490>
Any reason not to call expired() instead?
src/detector/detector.cpp
<https://reviews.apache.org/r/7295/#comment25491>
prematurely
src/detector/detector.cpp
<https://reviews.apache.org/r/7295/#comment25494>
timer needs to be cancelled and set to none so that we don't double expire here on a pending timeout.
src/detector/detector.cpp
<https://reviews.apache.org/r/7295/#comment25493>
expired = true;
src/detector/detector.cpp
<https://reviews.apache.org/r/7295/#comment25492>
Why not directly expire() here instead of marking state and then waiting for connected() before finally expire()ing ?
- John Sirois
On Sept. 26, 2012, 6:01 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7295/
> -----------------------------------------------------------
>
> (Updated Sept. 26, 2012, 6:01 p.m.)
>
>
> Review request for mesos, John Sirois, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary (7 of 7).
>
>
> Diffs
> -----
>
> src/detector/detector.cpp 0246846
> third_party/libprocess/include/process/delay.hpp cb2fa9a
> third_party/libprocess/include/process/timer.hpp 333a806
> third_party/libprocess/src/process.cpp fde7154
>
> Diff: https://reviews.apache.org/r/7295/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Changed the ZooKeeper master detector to "time out" a
session for instances that are being used to contend for leadership (and
send a NoMasterDetectedMessage and expire the session after a reconnect
does occur).
Posted by John Sirois <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7295/#review12113
-----------------------------------------------------------
Ship it!
Ship It!
- John Sirois
On Oct. 2, 2012, 7:12 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7295/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2012, 7:12 p.m.)
>
>
> Review request for mesos, John Sirois, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary (7 of 7).
>
>
> Diffs
> -----
>
> src/detector/detector.cpp 0246846
>
> Diff: https://reviews.apache.org/r/7295/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Changed the ZooKeeper master detector to "time out" a
session for instances that are being used to contend for leadership (and
send a NoMasterDetectedMessage and expire the session after a reconnect
does occur).
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7295/
-----------------------------------------------------------
(Updated Oct. 2, 2012, 7:12 p.m.)
Review request for mesos, John Sirois, Vinod Kone, and Ben Mahler.
Changes
-------
Updates based on review comments.
Description
-------
See summary (7 of 7).
Diffs (updated)
-----
src/detector/detector.cpp 0246846
Diff: https://reviews.apache.org/r/7295/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request: Changed the ZooKeeper master detector to "time out" a
session for instances that are being used to contend for leadership (and
send a NoMasterDetectedMessage and expire the session after a reconnect
does occur).
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Sept. 28, 2012, 12:38 a.m., John Sirois wrote:
> > src/detector/detector.cpp, line 480
> > <https://reviews.apache.org/r/7295/diff/1-4/?file=160216#file160216line480>
> >
> > you have sessionId in hand now and it may be useful to have it logged in this message for correlation with zk server logs in some unforeseen spelunk.
Done.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7295/#review12011
-----------------------------------------------------------
On Sept. 27, 2012, 10:02 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7295/
> -----------------------------------------------------------
>
> (Updated Sept. 27, 2012, 10:02 p.m.)
>
>
> Review request for mesos, John Sirois, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary (7 of 7).
>
>
> Diffs
> -----
>
> src/detector/detector.cpp 0246846
>
> Diff: https://reviews.apache.org/r/7295/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Changed the ZooKeeper master detector to "time out" a
session for instances that are being used to contend for leadership (and
send a NoMasterDetectedMessage and expire the session after a reconnect
does occur).
Posted by John Sirois <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7295/#review12011
-----------------------------------------------------------
Ship it!
src/detector/detector.cpp
<https://reviews.apache.org/r/7295/#comment25615>
you have sessionId in hand now and it may be useful to have it logged in this message for correlation with zk server logs in some unforeseen spelunk.
- John Sirois
On Sept. 27, 2012, 10:02 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7295/
> -----------------------------------------------------------
>
> (Updated Sept. 27, 2012, 10:02 p.m.)
>
>
> Review request for mesos, John Sirois, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary (7 of 7).
>
>
> Diffs
> -----
>
> src/detector/detector.cpp 0246846
>
> Diff: https://reviews.apache.org/r/7295/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Changed the ZooKeeper master detector to "time out" a
session for instances that are being used to contend for leadership (and
send a NoMasterDetectedMessage and expire the session after a reconnect
does occur).
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Sept. 27, 2012, 10:07 p.m., Ben Mahler wrote:
> > src/detector/detector.cpp, line 401
> > <https://reviews.apache.org/r/7295/diff/2/?file=160372#file160372line401>
> >
> > Maybe it's just me but I find structure makes these big paragraph comments easier to read:
> >
> > // Okay, we didn't prematurally expire the session, so either this
> > // detector was contending to be a master or not.
> > // If it was not, we want to run an election.
> > // If it was, either:
> > // 1. "We're" still the master (because "we" haven't
> > // yet gotten a session expiration).
> > // 2. Someone else was the master and they're still the master.
> > // 3. Someone else was the master and someone else still is now the master.
> > // Either way, run an election.
I dramatically simplified the comment.
> On Sept. 27, 2012, 10:07 p.m., Ben Mahler wrote:
> > src/detector/detector.cpp, line 389
> > <https://reviews.apache.org/r/7295/diff/2/?file=160372#file160372line389>
> >
> > google-glog comes with CHECK_NOTNULL
> >
> > you can then one-line it, if you like:
> > delete CHECK_NOTNULL(watcher);
Great, also fixed a bug in delete ordering, thanks!
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7295/#review12005
-----------------------------------------------------------
On Sept. 27, 2012, 10:02 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7295/
> -----------------------------------------------------------
>
> (Updated Sept. 27, 2012, 10:02 p.m.)
>
>
> Review request for mesos, John Sirois, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary (7 of 7).
>
>
> Diffs
> -----
>
> src/detector/detector.cpp 0246846
>
> Diff: https://reviews.apache.org/r/7295/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Changed the ZooKeeper master detector to "time out" a
session for instances that are being used to contend for leadership (and
send a NoMasterDetectedMessage and expire the session after a reconnect
does occur).
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7295/#review12005
-----------------------------------------------------------
src/detector/detector.cpp
<https://reviews.apache.org/r/7295/#comment25582>
The comment is good, but it's a run-on sentence (had to read it a few times to get it).
src/detector/detector.cpp
<https://reviews.apache.org/r/7295/#comment25583>
google-glog comes with CHECK_NOTNULL
you can then one-line it, if you like:
delete CHECK_NOTNULL(watcher);
src/detector/detector.cpp
<https://reviews.apache.org/r/7295/#comment25584>
ditto
src/detector/detector.cpp
<https://reviews.apache.org/r/7295/#comment25585>
Maybe it's just me but I find structure makes these big paragraph comments easier to read:
// Okay, we didn't prematurally expire the session, so either this
// detector was contending to be a master or not.
// If it was not, we want to run an election.
// If it was, either:
// 1. "We're" still the master (because "we" haven't
// yet gotten a session expiration).
// 2. Someone else was the master and they're still the master.
// 3. Someone else was the master and someone else still is now the master.
// Either way, run an election.
src/detector/detector.cpp
<https://reviews.apache.org/r/7295/#comment25586>
this comment reads better than the other big ones, so maybe it's just run-ons that I find hard to immediately grok
src/detector/detector.cpp
<https://reviews.apache.org/r/7295/#comment25587>
ditto CHECK_NOTNULL
src/detector/detector.cpp
<https://reviews.apache.org/r/7295/#comment25588>
ditto
src/detector/detector.cpp
<https://reviews.apache.org/r/7295/#comment25589>
ditto
third_party/libprocess/include/process/timer.hpp
<https://reviews.apache.org/r/7295/#comment25590>
curious why we called this thunk?
- Ben Mahler
On Sept. 27, 2012, 10:02 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7295/
> -----------------------------------------------------------
>
> (Updated Sept. 27, 2012, 10:02 p.m.)
>
>
> Review request for mesos, John Sirois, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary (7 of 7).
>
>
> Diffs
> -----
>
> src/detector/detector.cpp 0246846
>
> Diff: https://reviews.apache.org/r/7295/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Changed the ZooKeeper master detector to "time out" a
session for instances that are being used to contend for leadership (and
send a NoMasterDetectedMessage and expire the session after a reconnect
does occur).
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7295/#review12029
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On Sept. 27, 2012, 10:02 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7295/
> -----------------------------------------------------------
>
> (Updated Sept. 27, 2012, 10:02 p.m.)
>
>
> Review request for mesos, John Sirois, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary (7 of 7).
>
>
> Diffs
> -----
>
> src/detector/detector.cpp 0246846
>
> Diff: https://reviews.apache.org/r/7295/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Changed the ZooKeeper master detector to "time out" a
session for instances that are being used to contend for leadership (and
send a NoMasterDetectedMessage and expire the session after a reconnect
does occur).
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7295/
-----------------------------------------------------------
(Updated Sept. 27, 2012, 10:02 p.m.)
Review request for mesos, John Sirois, Vinod Kone, and Ben Mahler.
Changes
-------
Removed unnecessary process::delay and process::Timer changes.
Description
-------
See summary (7 of 7).
Diffs (updated)
-----
src/detector/detector.cpp 0246846
Diff: https://reviews.apache.org/r/7295/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request: Changed the ZooKeeper master detector to "time out" a
session for instances that are being used to contend for leadership (and
send a NoMasterDetectedMessage and expire the session after a reconnect
does occur).
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7295/
-----------------------------------------------------------
(Updated Sept. 27, 2012, 9:52 p.m.)
Review request for mesos, John Sirois, Vinod Kone, and Ben Mahler.
Changes
-------
Neglected to actually commit all review comment updates.
Description
-------
See summary (7 of 7).
Diffs (updated)
-----
src/detector/detector.cpp 0246846
third_party/libprocess/include/process/delay.hpp cb2fa9a
third_party/libprocess/include/process/timer.hpp 333a806
third_party/libprocess/src/process.cpp fde7154
Diff: https://reviews.apache.org/r/7295/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request: Changed the ZooKeeper master detector to "time out" a
session for instances that are being used to contend for leadership (and
send a NoMasterDetectedMessage and expire the session after a reconnect
does occur).
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7295/
-----------------------------------------------------------
(Updated Sept. 27, 2012, 9:46 p.m.)
Review request for mesos, John Sirois, Vinod Kone, and Ben Mahler.
Changes
-------
Updated diffs based on review comments.
Description
-------
See summary (7 of 7).
Diffs (updated)
-----
src/detector/detector.cpp 0246846
third_party/libprocess/include/process/delay.hpp cb2fa9a
third_party/libprocess/include/process/timer.hpp 333a806
third_party/libprocess/src/process.cpp fde7154
Diff: https://reviews.apache.org/r/7295/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request: Changed the ZooKeeper master detector to "time out" a
session for instances that are being used to contend for leadership (and
send a NoMasterDetectedMessage and expire the session after a reconnect
does occur).
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On Sept. 26, 2012, 9:36 p.m., Vinod Kone wrote:
> > src/detector/detector.cpp, line 398
> > <https://reviews.apache.org/r/7295/diff/1/?file=160216#file160216line398>
> >
> > why do you want run an election if it was not contending? am i reading it wrong, or did you flip the former with latter
We were always "running an election" for non-contending instances of the detector. Note that "running an election" == invoking detectMaster.
> On Sept. 26, 2012, 9:36 p.m., Vinod Kone wrote:
> > src/detector/detector.cpp, line 408
> > <https://reviews.apache.org/r/7295/diff/1/?file=160216#file160216line408>
> >
> > do we get multiple of these events as the client library keeps re-trying or only once?
Only one.
> On Sept. 26, 2012, 9:36 p.m., Vinod Kone wrote:
> > src/detector/detector.cpp, line 466
> > <https://reviews.apache.org/r/7295/diff/1/?file=160216#file160216line466>
> >
> > we need timer.isSome() because there is a race between Timer::cancel and we receiving this timedout message?
> >
> > again, why do we need the timeout variable here? is it because, we are expecting multiple timers to be in flight at the same time?
Yes, multiple possible timers. But I decided to pass the session ID instead (see the updated diff).
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7295/#review11949
-----------------------------------------------------------
On Sept. 26, 2012, 6:01 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7295/
> -----------------------------------------------------------
>
> (Updated Sept. 26, 2012, 6:01 p.m.)
>
>
> Review request for mesos, John Sirois, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary (7 of 7).
>
>
> Diffs
> -----
>
> src/detector/detector.cpp 0246846
> third_party/libprocess/include/process/delay.hpp cb2fa9a
> third_party/libprocess/include/process/timer.hpp 333a806
> third_party/libprocess/src/process.cpp fde7154
>
> Diff: https://reviews.apache.org/r/7295/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request: Changed the ZooKeeper master detector to "time out" a
session for instances that are being used to contend for leadership (and
send a NoMasterDetectedMessage and expire the session after a reconnect
does occur).
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7295/#review11949
-----------------------------------------------------------
src/detector/detector.cpp
<https://reviews.apache.org/r/7295/#comment25481>
why do you want run an election if it was not contending? am i reading it wrong, or did you flip the former with latter
src/detector/detector.cpp
<https://reviews.apache.org/r/7295/#comment25483>
do we get multiple of these events as the client library keeps re-trying or only once?
src/detector/detector.cpp
<https://reviews.apache.org/r/7295/#comment25480>
s/a/at
src/detector/detector.cpp
<https://reviews.apache.org/r/7295/#comment25482>
why not do this for non-leading contenders too?
src/detector/detector.cpp
<https://reviews.apache.org/r/7295/#comment25484>
we need timer.isSome() because there is a race between Timer::cancel and we receiving this timedout message?
again, why do we need the timeout variable here? is it because, we are expecting multiple timers to be in flight at the same time?
- Vinod Kone
On Sept. 26, 2012, 6:01 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7295/
> -----------------------------------------------------------
>
> (Updated Sept. 26, 2012, 6:01 p.m.)
>
>
> Review request for mesos, John Sirois, Vinod Kone, and Ben Mahler.
>
>
> Description
> -------
>
> See summary (7 of 7).
>
>
> Diffs
> -----
>
> src/detector/detector.cpp 0246846
> third_party/libprocess/include/process/delay.hpp cb2fa9a
> third_party/libprocess/include/process/timer.hpp 333a806
> third_party/libprocess/src/process.cpp fde7154
>
> Diff: https://reviews.apache.org/r/7295/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>