You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jiang Yan Xu <ya...@jxu.me> on 2013/10/03 07:51:00 UTC
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/
-----------------------------------------------------------
(Updated Oct. 3, 2013, 5:50 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Revised the API to a "Result previous" instead of "bool force" as a parameter so the caller of the detector does not need to know when to pass "force=true". See comments in https://reviews.apache.org/r/13087/
Bugs: MESOS-496
https://issues.apache.org/jira/browse/MESOS-496
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
src/Makefile.am ee336130ad93d8b524c841f75be36f00d4a2b147
src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
src/zookeeper/contender.hpp PRE-CREATION
src/zookeeper/contender.cpp PRE-CREATION
src/zookeeper/detector.hpp PRE-CREATION
src/zookeeper/detector.cpp PRE-CREATION
src/zookeeper/group.hpp 6b6c1557658df1bb7b7df3ded5c203e2dc0c8308
src/zookeeper/group.cpp cd58d230a2715b9ed01002443ebd7b5366d31be6
Diff: https://reviews.apache.org/r/13086/diff/
Testing
-------
make check 100 times.
Thanks,
Jiang Yan Xu
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On Oct. 28, 2013, 5:20 a.m., Vinod Kone wrote:
> > src/zookeeper/contender.cpp, lines 190-193
> > <https://reviews.apache.org/r/13086/diff/8/?file=371404#file371404line190>
> >
> > How is watching.isSome() possible?
> >
> > I am also not sure how withdrawing.isSome() is possible.
withdrawing.isSome() when the user calls withdraw after calling contending() and before joined() is called.
I removed "watching.isSome()" as this was no longer possible. In the past it was the complication of reusability of the contender.
> On Oct. 28, 2013, 5:20 a.m., Vinod Kone wrote:
> > src/zookeeper/group.cpp, lines 233-235
> > <https://reviews.apache.org/r/13086/diff/8/?file=371408#file371408line233>
> >
> > why do we need a timer on join?
There were added because the client could be calling join() after we have locally timed out the session and recreated the ZK handle but it still cannot get connected. In this case there is no running timer because it already ran and timed out. As a result the operation is pending forever unless the Group reconnects.
With the added timeouts all operations fail if Group cannot reconnect to ZK within the local timeout.
I now have killed all these after communicating with Vinod offline and in such a case we just let the operations pend.
> On Oct. 28, 2013, 5:20 a.m., Vinod Kone wrote:
> > src/zookeeper/group.cpp, line 497
> > <https://reviews.apache.org/r/13086/diff/8/?file=371408#file371408line497>
> >
> > I might've asked this earlier, but why abort() instead of expired()? iiuc, the time out is just fast forwarding the expiration process, no?
timeout is fast forwarding the expiration process; it does not fail pending operations. In fact, pending operation can still be executed after Group reconnects. Here the goal is to hide the expiration.
abort() fails all pending operations and expires the session. Here we want to the inform the clients that we are locally terminating the session (aborting), we do not hide it because the client might not want to wait indefinitely.
> On Oct. 28, 2013, 5:20 a.m., Vinod Kone wrote:
> > src/zookeeper/group.cpp, line 869
> > <https://reviews.apache.org/r/13086/diff/8/?file=371408#file371408line869>
> >
> > Why this reset here?
Because after this abort/reset, the group is expected to be reused (even before it reconnects, in which case the operations are queued). Resetting the error allows Group operations (join, withdraw, cancel) to be callable again.
> On Oct. 28, 2013, 5:20 a.m., Vinod Kone wrote:
> > src/zookeeper/contender.cpp, line 217
> > <https://reviews.apache.org/r/13086/diff/8/?file=371404#file371404line217>
> >
> > Why do you need to store 'memberships'. It looks like it is only used in watched(). So can't we just pass it via onAny?
We discard memberships in abort()
> On Oct. 28, 2013, 5:20 a.m., Vinod Kone wrote:
> > src/zookeeper/group.cpp, lines 322-324
> > <https://reviews.apache.org/r/13086/diff/8/?file=371408#file371408line322>
> >
> > Why here?
Comments above.
> On Oct. 28, 2013, 5:20 a.m., Vinod Kone wrote:
> > src/zookeeper/contender.cpp, line 201
> > <https://reviews.apache.org/r/13086/diff/8/?file=371404#file371404line201>
> >
> > Why do you even need to store 'membership' when you can use 'candidacy'?
It was meant for clarity but seemed not achieving the goal... Killed 'membership'.
> On Oct. 28, 2013, 5:20 a.m., Vinod Kone wrote:
> > src/zookeeper/contender.cpp, lines 152-153
> > <https://reviews.apache.org/r/13086/diff/8/?file=371404#file371404line152>
> >
> > Shouldn't we eventually withdraw after contending succeeds?
Made it return a failure in such case:
if (contending.isNone() || !candidacy.isReady()) {
return Future<bool>::failed(
"Can only withdraw after the contender has contended");
}
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/#review27586
-----------------------------------------------------------
On Oct. 25, 2013, 5:46 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13086/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2013, 5:46 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-496
> https://issues.apache.org/jira/browse/MESOS-496
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am a2d82425f74dcaf3adf28ae8184fff53b3c34ceb
> src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
> src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
> src/zookeeper/contender.hpp PRE-CREATION
> src/zookeeper/contender.cpp PRE-CREATION
> src/zookeeper/detector.hpp PRE-CREATION
> src/zookeeper/detector.cpp PRE-CREATION
> src/zookeeper/group.hpp 6b6c1557658df1bb7b7df3ded5c203e2dc0c8308
> src/zookeeper/group.cpp cd58d230a2715b9ed01002443ebd7b5366d31be6
>
> Diff: https://reviews.apache.org/r/13086/diff/
>
>
> Testing
> -------
>
> make check 100 times.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/#review27586
-----------------------------------------------------------
This is looking good! I think we are getting close to get this committed. Thanks for the iterations.
My main comments are:
--> I think we can get rid of couple instance variables in contender.
--> Lets split the group changes in to their own review. IIUC, the main goal there is to add a time out.
src/zookeeper/contender.hpp
<https://reviews.apache.org/r/13086/#comment53597>
s/Creates a new leader contender instance.//
src/zookeeper/contender.hpp
<https://reviews.apache.org/r/13086/#comment53598>
s/Destroys the contender object.//
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment53599>
Kill this statement.
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment53600>
Kill this.
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment53602>
Why optional?
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment53601>
How about
"contend() can only be called once." ?
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment53604>
Shouldn't we eventually withdraw after contending succeeds?
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment53606>
LOG?
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment53607>
How is watching.isSome() possible?
I am also not sure how withdrawing.isSome() is possible.
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment53610>
Why do you even need to store 'membership' when you can use 'candidacy'?
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment53612>
Why do you need to store 'memberships'. It looks like it is only used in watched(). So can't we just pass it via onAny?
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment53611>
LOG?
src/zookeeper/detector.hpp
<https://reviews.apache.org/r/13086/#comment53613>
s/Creates a new leader detector instance.//
src/zookeeper/group.cpp
<https://reviews.apache.org/r/13086/#comment53614>
why do we need a timer on join?
src/zookeeper/group.cpp
<https://reviews.apache.org/r/13086/#comment53615>
Why here?
src/zookeeper/group.cpp
<https://reviews.apache.org/r/13086/#comment53617>
I might've asked this earlier, but why abort() instead of expired()? iiuc, the time out is just fast forwarding the expiration process, no?
src/zookeeper/group.cpp
<https://reviews.apache.org/r/13086/#comment53616>
Why this reset here?
- Vinod Kone
On Oct. 25, 2013, 5:46 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13086/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2013, 5:46 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-496
> https://issues.apache.org/jira/browse/MESOS-496
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am a2d82425f74dcaf3adf28ae8184fff53b3c34ceb
> src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
> src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
> src/zookeeper/contender.hpp PRE-CREATION
> src/zookeeper/contender.cpp PRE-CREATION
> src/zookeeper/detector.hpp PRE-CREATION
> src/zookeeper/detector.cpp PRE-CREATION
> src/zookeeper/group.hpp 6b6c1557658df1bb7b7df3ded5c203e2dc0c8308
> src/zookeeper/group.cpp cd58d230a2715b9ed01002443ebd7b5366d31be6
>
> Diff: https://reviews.apache.org/r/13086/diff/
>
>
> Testing
> -------
>
> make check 100 times.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/#review27698
-----------------------------------------------------------
i still think memberships could be eliminated as discussed offline.
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment53784>
const.
- Vinod Kone
On Oct. 29, 2013, 7:20 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13086/
> -----------------------------------------------------------
>
> (Updated Oct. 29, 2013, 7:20 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-496
> https://issues.apache.org/jira/browse/MESOS-496
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am 0b32d74e43d21eb2f77b0abd8864ba5f8cf46214
> src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
> src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
> src/zookeeper/contender.hpp PRE-CREATION
> src/zookeeper/contender.cpp PRE-CREATION
> src/zookeeper/detector.hpp PRE-CREATION
> src/zookeeper/detector.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/13086/diff/
>
>
> Testing
> -------
>
> make check 100 times.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/#review27792
-----------------------------------------------------------
Ship it!
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment54042>
i think obtained is better.
src/zookeeper/detector.cpp
<https://reviews.apache.org/r/13086/#comment54043>
Kill this?
- Vinod Kone
On Oct. 30, 2013, 5:13 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13086/
> -----------------------------------------------------------
>
> (Updated Oct. 30, 2013, 5:13 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-496
> https://issues.apache.org/jira/browse/MESOS-496
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am a11c76b998ccc84daca02facddd20a4acb88aa43
> src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
> src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
> src/zookeeper/contender.hpp PRE-CREATION
> src/zookeeper/contender.cpp PRE-CREATION
> src/zookeeper/detector.hpp PRE-CREATION
> src/zookeeper/detector.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/13086/diff/
>
>
> Testing
> -------
>
> make check 100 times.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/#review28689
-----------------------------------------------------------
Ship it!
Looks good, the two things were:
1. Using .then on candidacy to withdraw when a caller calls withdraw() before achieving candidacy.
2. Let's think about what we do on discard operations, since we're not withdrawing maybe it's better to do nothing like Group?
- Ben Mahler
On Nov. 5, 2013, 10:17 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13086/
> -----------------------------------------------------------
>
> (Updated Nov. 5, 2013, 10:17 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-496
> https://issues.apache.org/jira/browse/MESOS-496
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am 9780d07a23ca196c541a44a85499c2f44a574b9c
> src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
> src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
> src/zookeeper/contender.hpp PRE-CREATION
> src/zookeeper/contender.cpp PRE-CREATION
> src/zookeeper/detector.hpp PRE-CREATION
> src/zookeeper/detector.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/13086/diff/
>
>
> Testing
> -------
>
> make check 100 times.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/#review28878
-----------------------------------------------------------
Ship it!
src/tests/zookeeper_tests.cpp
<https://reviews.apache.org/r/13086/#comment55944>
new line.
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment55947>
s/Repeatedly/Repeated/
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment55948>
CHECK(contending.isReady()) << ".." ; ?
- Vinod Kone
On Nov. 14, 2013, 8:23 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13086/
> -----------------------------------------------------------
>
> (Updated Nov. 14, 2013, 8:23 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-496
> https://issues.apache.org/jira/browse/MESOS-496
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am e6f7120a9fa014ea1bbbdc3c0ed305abbfe0e741
> src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
> src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
> src/zookeeper/contender.hpp PRE-CREATION
> src/zookeeper/contender.cpp PRE-CREATION
> src/zookeeper/detector.hpp PRE-CREATION
> src/zookeeper/detector.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/13086/diff/
>
>
> Testing
> -------
>
> make check 100 times.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/
-----------------------------------------------------------
(Updated Nov. 14, 2013, 7:48 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Minor mistake...
Bugs: MESOS-496
https://issues.apache.org/jira/browse/MESOS-496
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
src/Makefile.am e6f7120a9fa014ea1bbbdc3c0ed305abbfe0e741
src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
src/zookeeper/contender.hpp PRE-CREATION
src/zookeeper/contender.cpp PRE-CREATION
src/zookeeper/detector.hpp PRE-CREATION
src/zookeeper/detector.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/13086/diff/
Testing
-------
make check 100 times.
Thanks,
Jiang Yan Xu
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/
-----------------------------------------------------------
(Updated Nov. 14, 2013, 7:08 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Vinod's comments
Bugs: MESOS-496
https://issues.apache.org/jira/browse/MESOS-496
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
src/Makefile.am e6f7120a9fa014ea1bbbdc3c0ed305abbfe0e741
src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
src/zookeeper/contender.hpp PRE-CREATION
src/zookeeper/contender.cpp PRE-CREATION
src/zookeeper/detector.hpp PRE-CREATION
src/zookeeper/detector.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/13086/diff/
Testing
-------
make check 100 times.
Thanks,
Jiang Yan Xu
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/
-----------------------------------------------------------
(Updated Nov. 14, 2013, 8:23 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
BenM's comments.
Now:
- Calling contend() repeatedly: CHECK failed.
- Calling withdraw() repeatedly: return the same future.
- Withdraw() after contend() but before achieving candidacy: withdraw is delayed to after candidacy is achieved.
- Client discarding futures now do not trigger aborts.
Bugs: MESOS-496
https://issues.apache.org/jira/browse/MESOS-496
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
src/Makefile.am e6f7120a9fa014ea1bbbdc3c0ed305abbfe0e741
src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
src/zookeeper/contender.hpp PRE-CREATION
src/zookeeper/contender.cpp PRE-CREATION
src/zookeeper/detector.hpp PRE-CREATION
src/zookeeper/detector.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/13086/diff/
Testing
-------
make check 100 times.
Thanks,
Jiang Yan Xu
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/#review28687
-----------------------------------------------------------
Ship it!
- Ben Mahler
On Nov. 5, 2013, 10:17 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13086/
> -----------------------------------------------------------
>
> (Updated Nov. 5, 2013, 10:17 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-496
> https://issues.apache.org/jira/browse/MESOS-496
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am 9780d07a23ca196c541a44a85499c2f44a574b9c
> src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
> src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
> src/zookeeper/contender.hpp PRE-CREATION
> src/zookeeper/contender.cpp PRE-CREATION
> src/zookeeper/detector.hpp PRE-CREATION
> src/zookeeper/detector.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/13086/diff/
>
>
> Testing
> -------
>
> make check 100 times.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On Nov. 9, 2013, 1:21 a.m., Ben Mahler wrote:
> > src/zookeeper/contender.cpp, line 130
> > <https://reviews.apache.org/r/13086/diff/12/?file=378503#file378503line130>
> >
> > If candidacy is not ready, it's still possible that contend() has been called (if the group->join is still in progress). This doesn't match the documentation in contender.hpp:
> >
> > // Returns true if successfully withdrawn from the contest (either
> > // while contending or has already contended and is watching for
> > // membership loss).
> > // A false return value implies that there was no valid group
> > // membership to cancel, which may be a result of a race to cancel
> > // an expired membership.
> > // A failed future is returned if the method is called before
> > // contend() or more than once.
> > process::Future<bool> withdraw();
> >
> > In the case where contend() has been called, and withdraw() was called prior to us joining the Group, we'll be returning a failed Future?
>
> Ben Mahler wrote:
> When !candidacy.isReady(), can we just defer() back into withdraw without changing anything else?
> On Nov. 9, 2013, 1:21 a.m., Ben Mahler wrote:
> > src/zookeeper/detector.hpp, line 17
> > <https://reviews.apache.org/r/13086/diff/12/?file=378504#file378504line17>
> >
> > s/a "leader"/the leader/
> >
> > I wonder if we need the quotes around "leader" here and in contender? Seems to leave an open question around the meaning of leader.
Removed quotes here and in the contender.
> On Nov. 9, 2013, 1:21 a.m., Ben Mahler wrote:
> > src/zookeeper/detector.cpp, line 141
> > <https://reviews.apache.org/r/13086/diff/12/?file=378505#file378505line141>
> >
> > Does s/Result<Group::Membership>::none()/None()/ work?
No:
../../src/zookeeper/detector.cpp:139: error: call of overloaded 'set(None)' is ambiguous
../../3rdparty/libprocess/include/process/future.hpp:305: note: candidates are: bool process::Promise<T>::set(const T&) [with T = Result<zookeeper::Group::Membership>]
../../3rdparty/libprocess/include/process/future.hpp:312: note: bool process::Promise<T>::set(const process::Future<T>&) [with T = Result<zookeeper::Group::Membership>]
> On Nov. 9, 2013, 1:21 a.m., Ben Mahler wrote:
> > src/zookeeper/detector.cpp, line 65
> > <https://reviews.apache.org/r/13086/diff/12/?file=378505#file378505line65>
> >
> > Is the eager watch simpler than watching lazily?
I don't think so but here the asynchronicity speeds things up because obtains the leader data (i.e. PID) involves two calls to the Group so I'd better not rely on Group's background sync.
> On Nov. 9, 2013, 1:21 a.m., Ben Mahler wrote:
> > src/zookeeper/detector.cpp, lines 74-79
> > <https://reviews.apache.org/r/13086/diff/12/?file=378505#file378505line74>
> >
> > My brain hurts ;)
> >
> > How about:
> >
> > if (leader.isError() != previous.isError() ||
> > leader.isNone() != previous.isNone() ||
> > leader.isSome() != previous.isSome()) {
> > return leader; // State change.
> > } else if (leader.isSome() && previous.isSome() &&
> > leader.get() != previous.get()) {
> > return leader; // Leadership change.
> > }
Ok...
> On Nov. 9, 2013, 1:21 a.m., Ben Mahler wrote:
> > src/zookeeper/contender.cpp, lines 107-110
> > <https://reviews.apache.org/r/13086/diff/12/?file=378503#file378503line107>
> >
> > Let's CHECK here instead since the caller is violating the intended usage.
> >
> > {
> > CHECK(contending.isNone()) << "Cannot contend more than once";
> >
> > ...
> > }
Done. Note this is different from withdraw() (the comment above) because when people contend repeatedly (they shouldn't) they are probably never expecting the previous results.
> On Nov. 9, 2013, 1:21 a.m., Ben Mahler wrote:
> > src/zookeeper/contender.hpp, line 51
> > <https://reviews.apache.org/r/13086/diff/12/?file=378502#file378502line51>
> >
> > Seems like calling withdraw twice is harmless? Should we just return true instead of a failed Future?
Now return the same future if called repeatedly.
> On Nov. 9, 2013, 1:21 a.m., Ben Mahler wrote:
> > src/zookeeper/contender.hpp, lines 43-51
> > <https://reviews.apache.org/r/13086/diff/12/?file=378502#file378502line43>
> >
> > Is there a difference between withdraw() and calling discard() on one of the Futures returned by contend?
> >
> > The implementation of LeaderContender appears to do different things depending on whether the contend was discarded or withdraw() was called. Is it possible to simplify this and have both operations go through the same code paths?
> >
> > It seems like withdraw can handle all possible valid withdrawal states and the onDiscarded callbacks can call withdraw instead of fail?
> >
> > Or is there a reason not to withdraw when the client discards the future?
>
> Ben Mahler wrote:
> On discarded, we should either do nothing or the right thing (withdraw). How about we do nothing, like Group, and just increase the robustness of withdraw to handle the race I mentioned below?
Done.
> On Nov. 9, 2013, 1:21 a.m., Ben Mahler wrote:
> > src/zookeeper/detector.cpp, line 35
> > <https://reviews.apache.org/r/13086/diff/12/?file=378505#file378505line35>
> >
> > Why was this needed? There's a typedef for this in Process. Should we be using Self in the contender as well?
Removed.
> On Nov. 9, 2013, 1:21 a.m., Ben Mahler wrote:
> > src/zookeeper/detector.hpp, line 35
> > <https://reviews.apache.org/r/13086/diff/12/?file=378504#file378504line35>
> >
> > Would love to see a todo here:
> >
> > TODO: Use a Stream abstraction instead.
Done.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/#review28588
-----------------------------------------------------------
On Nov. 14, 2013, 8:23 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13086/
> -----------------------------------------------------------
>
> (Updated Nov. 14, 2013, 8:23 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-496
> https://issues.apache.org/jira/browse/MESOS-496
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am e6f7120a9fa014ea1bbbdc3c0ed305abbfe0e741
> src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
> src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
> src/zookeeper/contender.hpp PRE-CREATION
> src/zookeeper/contender.cpp PRE-CREATION
> src/zookeeper/detector.hpp PRE-CREATION
> src/zookeeper/detector.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/13086/diff/
>
>
> Testing
> -------
>
> make check 100 times.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
Posted by Ben Mahler <be...@gmail.com>.
> On Nov. 9, 2013, 1:21 a.m., Ben Mahler wrote:
> > src/zookeeper/contender.cpp, line 130
> > <https://reviews.apache.org/r/13086/diff/12/?file=378503#file378503line130>
> >
> > If candidacy is not ready, it's still possible that contend() has been called (if the group->join is still in progress). This doesn't match the documentation in contender.hpp:
> >
> > // Returns true if successfully withdrawn from the contest (either
> > // while contending or has already contended and is watching for
> > // membership loss).
> > // A false return value implies that there was no valid group
> > // membership to cancel, which may be a result of a race to cancel
> > // an expired membership.
> > // A failed future is returned if the method is called before
> > // contend() or more than once.
> > process::Future<bool> withdraw();
> >
> > In the case where contend() has been called, and withdraw() was called prior to us joining the Group, we'll be returning a failed Future?
When !candidacy.isReady(), can we just defer() back into withdraw without changing anything else?
> On Nov. 9, 2013, 1:21 a.m., Ben Mahler wrote:
> > src/zookeeper/contender.hpp, lines 43-51
> > <https://reviews.apache.org/r/13086/diff/12/?file=378502#file378502line43>
> >
> > Is there a difference between withdraw() and calling discard() on one of the Futures returned by contend?
> >
> > The implementation of LeaderContender appears to do different things depending on whether the contend was discarded or withdraw() was called. Is it possible to simplify this and have both operations go through the same code paths?
> >
> > It seems like withdraw can handle all possible valid withdrawal states and the onDiscarded callbacks can call withdraw instead of fail?
> >
> > Or is there a reason not to withdraw when the client discards the future?
On discarded, we should either do nothing or the right thing (withdraw). How about we do nothing, like Group, and just increase the robustness of withdraw to handle the race I mentioned below?
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/#review28588
-----------------------------------------------------------
On Nov. 5, 2013, 10:17 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13086/
> -----------------------------------------------------------
>
> (Updated Nov. 5, 2013, 10:17 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-496
> https://issues.apache.org/jira/browse/MESOS-496
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am 9780d07a23ca196c541a44a85499c2f44a574b9c
> src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
> src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
> src/zookeeper/contender.hpp PRE-CREATION
> src/zookeeper/contender.cpp PRE-CREATION
> src/zookeeper/detector.hpp PRE-CREATION
> src/zookeeper/detector.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/13086/diff/
>
>
> Testing
> -------
>
> make check 100 times.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/#review28588
-----------------------------------------------------------
Looks good, I mostly just had a question about the semantics in LeaderContender being different between discards and withdrawal.
src/zookeeper/contender.hpp
<https://reviews.apache.org/r/13086/#comment55427>
Is there a difference between withdraw() and calling discard() on one of the Futures returned by contend?
The implementation of LeaderContender appears to do different things depending on whether the contend was discarded or withdraw() was called. Is it possible to simplify this and have both operations go through the same code paths?
It seems like withdraw can handle all possible valid withdrawal states and the onDiscarded callbacks can call withdraw instead of fail?
Or is there a reason not to withdraw when the client discards the future?
src/zookeeper/contender.hpp
<https://reviews.apache.org/r/13086/#comment55444>
Seems like calling withdraw twice is harmless? Should we just return true instead of a failed Future?
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment55432>
Let's CHECK here instead since the caller is violating the intended usage.
{
CHECK(contending.isNone()) << "Cannot contend more than once";
...
}
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment55436>
'" << data << "'";
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment55435>
If candidacy is not ready, it's still possible that contend() has been called (if the group->join is still in progress). This doesn't match the documentation in contender.hpp:
// Returns true if successfully withdrawn from the contest (either
// while contending or has already contended and is watching for
// membership loss).
// A false return value implies that there was no valid group
// membership to cancel, which may be a result of a race to cancel
// an expired membership.
// A failed future is returned if the method is called before
// contend() or more than once.
process::Future<bool> withdraw();
In the case where contend() has been called, and withdraw() was called prior to us joining the Group, we'll be returning a failed Future?
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment55441>
Looks like you're not expecting memberships to be discarded, mind adding a CHECK for this?
src/zookeeper/detector.hpp
<https://reviews.apache.org/r/13086/#comment55447>
s/a "leader"/the leader/
I wonder if we need the quotes around "leader" here and in contender? Seems to leave an open question around the meaning of leader.
src/zookeeper/detector.hpp
<https://reviews.apache.org/r/13086/#comment55448>
Would love to see a todo here:
TODO: Use a Stream abstraction instead.
src/zookeeper/detector.cpp
<https://reviews.apache.org/r/13086/#comment55449>
Why was this needed? There's a typedef for this in Process. Should we be using Self in the contender as well?
src/zookeeper/detector.cpp
<https://reviews.apache.org/r/13086/#comment55465>
How about making this a queue so we can pop off promises when we set them, akin to what is done in group.cpp?
src/zookeeper/detector.cpp
<https://reviews.apache.org/r/13086/#comment55450>
Is the eager watch simpler than watching lazily?
src/zookeeper/detector.cpp
<https://reviews.apache.org/r/13086/#comment55454>
My brain hurts ;)
How about:
if (leader.isError() != previous.isError() ||
leader.isNone() != previous.isNone() ||
leader.isSome() != previous.isSome()) {
return leader; // State change.
} else if (leader.isSome() && previous.isSome() &&
leader.get() != previous.get()) {
return leader; // Leadership change.
}
src/zookeeper/detector.cpp
<https://reviews.apache.org/r/13086/#comment55466>
"was lost"
src/zookeeper/detector.cpp
<https://reviews.apache.org/r/13086/#comment55467>
Does s/Result<Group::Membership>::none()/None()/ work?
- Ben Mahler
On Nov. 5, 2013, 10:17 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13086/
> -----------------------------------------------------------
>
> (Updated Nov. 5, 2013, 10:17 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
>
>
> Bugs: MESOS-496
> https://issues.apache.org/jira/browse/MESOS-496
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am 9780d07a23ca196c541a44a85499c2f44a574b9c
> src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
> src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
> src/zookeeper/contender.hpp PRE-CREATION
> src/zookeeper/contender.cpp PRE-CREATION
> src/zookeeper/detector.hpp PRE-CREATION
> src/zookeeper/detector.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/13086/diff/
>
>
> Testing
> -------
>
> make check 100 times.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/
-----------------------------------------------------------
(Updated Nov. 5, 2013, 10:17 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Explicitly compare Results because I removed the != operator in Result.
Bugs: MESOS-496
https://issues.apache.org/jira/browse/MESOS-496
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
src/Makefile.am 9780d07a23ca196c541a44a85499c2f44a574b9c
src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
src/zookeeper/contender.hpp PRE-CREATION
src/zookeeper/contender.cpp PRE-CREATION
src/zookeeper/detector.hpp PRE-CREATION
src/zookeeper/detector.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/13086/diff/
Testing
-------
make check 100 times.
Thanks,
Jiang Yan Xu
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/
-----------------------------------------------------------
(Updated Nov. 5, 2013, 1:09 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Moved the call to watch(set<Group::Membership>()) from within the constructor to initialize()
Bugs: MESOS-496
https://issues.apache.org/jira/browse/MESOS-496
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
src/Makefile.am 9780d07a23ca196c541a44a85499c2f44a574b9c
src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
src/zookeeper/contender.hpp PRE-CREATION
src/zookeeper/contender.cpp PRE-CREATION
src/zookeeper/detector.hpp PRE-CREATION
src/zookeeper/detector.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/13086/diff/
Testing
-------
make check 100 times.
Thanks,
Jiang Yan Xu
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/
-----------------------------------------------------------
(Updated Oct. 30, 2013, 5:13 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
- Killed 'memberships'
- Detector now use Result<T>'s "!=" operator to determine whether to directly return.
Bugs: MESOS-496
https://issues.apache.org/jira/browse/MESOS-496
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
src/Makefile.am a11c76b998ccc84daca02facddd20a4acb88aa43
src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
src/zookeeper/contender.hpp PRE-CREATION
src/zookeeper/contender.cpp PRE-CREATION
src/zookeeper/detector.hpp PRE-CREATION
src/zookeeper/detector.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/13086/diff/
Testing
-------
make check 100 times.
Thanks,
Jiang Yan Xu
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/
-----------------------------------------------------------
(Updated Oct. 29, 2013, 7:20 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Moved Group to a separate review.
Vinod's comments.
Bugs: MESOS-496
https://issues.apache.org/jira/browse/MESOS-496
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
src/Makefile.am 0b32d74e43d21eb2f77b0abd8864ba5f8cf46214
src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
src/zookeeper/contender.hpp PRE-CREATION
src/zookeeper/contender.cpp PRE-CREATION
src/zookeeper/detector.hpp PRE-CREATION
src/zookeeper/detector.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/13086/diff/
Testing
-------
make check 100 times.
Thanks,
Jiang Yan Xu
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/
-----------------------------------------------------------
(Updated Oct. 25, 2013, 5:46 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Bugs: MESOS-496
https://issues.apache.org/jira/browse/MESOS-496
Repository: mesos-git
Description
-------
See summary.
Diffs
-----
src/Makefile.am a2d82425f74dcaf3adf28ae8184fff53b3c34ceb
src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
src/zookeeper/contender.hpp PRE-CREATION
src/zookeeper/contender.cpp PRE-CREATION
src/zookeeper/detector.hpp PRE-CREATION
src/zookeeper/detector.cpp PRE-CREATION
src/zookeeper/group.hpp 6b6c1557658df1bb7b7df3ded5c203e2dc0c8308
src/zookeeper/group.cpp cd58d230a2715b9ed01002443ebd7b5366d31be6
Diff: https://reviews.apache.org/r/13086/diff/
Testing
-------
make check 100 times.
Thanks,
Jiang Yan Xu
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/
-----------------------------------------------------------
(Updated Oct. 25, 2013, 5:44 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, and Vinod Kone.
Changes
-------
Now Contender::withdraw() doesn't fail the future returned by contend().
Bugs: MESOS-496
https://issues.apache.org/jira/browse/MESOS-496
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
src/Makefile.am a2d82425f74dcaf3adf28ae8184fff53b3c34ceb
src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
src/zookeeper/contender.hpp PRE-CREATION
src/zookeeper/contender.cpp PRE-CREATION
src/zookeeper/detector.hpp PRE-CREATION
src/zookeeper/detector.cpp PRE-CREATION
src/zookeeper/group.hpp 6b6c1557658df1bb7b7df3ded5c203e2dc0c8308
src/zookeeper/group.cpp cd58d230a2715b9ed01002443ebd7b5366d31be6
Diff: https://reviews.apache.org/r/13086/diff/
Testing
-------
make check 100 times.
Thanks,
Jiang Yan Xu
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/
-----------------------------------------------------------
(Updated Oct. 24, 2013, 7:24 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone.
Changes
-------
Made contender non-reusable.
Bugs: MESOS-496
https://issues.apache.org/jira/browse/MESOS-496
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
src/Makefile.am a2d82425f74dcaf3adf28ae8184fff53b3c34ceb
src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
src/zookeeper/contender.hpp PRE-CREATION
src/zookeeper/contender.cpp PRE-CREATION
src/zookeeper/detector.hpp PRE-CREATION
src/zookeeper/detector.cpp PRE-CREATION
src/zookeeper/group.hpp 6b6c1557658df1bb7b7df3ded5c203e2dc0c8308
src/zookeeper/group.cpp cd58d230a2715b9ed01002443ebd7b5366d31be6
Diff: https://reviews.apache.org/r/13086/diff/
Testing
-------
make check 100 times.
Thanks,
Jiang Yan Xu
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/#review27287
-----------------------------------------------------------
src/zookeeper/detector.cpp
<https://reviews.apache.org/r/13086/#comment53123>
Changed to
if (leader.isSome() && (!previous.isSome() ||
leader.get().id() != previous.get().id())) {
return leader;
}
- Jiang Yan Xu
On Oct. 14, 2013, 11:30 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13086/
> -----------------------------------------------------------
>
> (Updated Oct. 14, 2013, 11:30 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-496
> https://issues.apache.org/jira/browse/MESOS-496
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am a2d82425f74dcaf3adf28ae8184fff53b3c34ceb
> src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
> src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
> src/zookeeper/contender.hpp PRE-CREATION
> src/zookeeper/contender.cpp PRE-CREATION
> src/zookeeper/detector.hpp PRE-CREATION
> src/zookeeper/detector.cpp PRE-CREATION
> src/zookeeper/group.hpp 6b6c1557658df1bb7b7df3ded5c203e2dc0c8308
> src/zookeeper/group.cpp cd58d230a2715b9ed01002443ebd7b5366d31be6
>
> Diff: https://reviews.apache.org/r/13086/diff/
>
>
> Testing
> -------
>
> make check 100 times.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote:
> > src/zookeeper/contender.cpp, lines 32-33
> > <https://reviews.apache.org/r/13086/diff/6/?file=364281#file364281line32>
> >
> > what is the difference?
Killed `virtual void initialize()`;
> On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote:
> > src/zookeeper/contender.cpp, line 42
> > <https://reviews.apache.org/r/13086/diff/6/?file=364281#file364281line42>
> >
> > how do you know you failed?
The future is not passed as a parameter but stored as a class variable.
> On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote:
> > src/zookeeper/contender.cpp, line 59
> > <https://reviews.apache.org/r/13086/diff/6/?file=364281#file364281line59>
> >
> > s/abort/cancel/?
> >
> > or
> >
> > s/abort/fail/ ?
s/abort/fail/
> On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote:
> > src/zookeeper/contender.cpp, lines 140-141
> > <https://reviews.apache.org/r/13086/diff/6/?file=364281#file364281line140>
> >
> > So, if we are already withdrawing we just return the future. Why not do the same when we are contending?
Ok.
> On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote:
> > src/zookeeper/contender.cpp, line 177
> > <https://reviews.apache.org/r/13086/diff/6/?file=364281#file364281line177>
> >
> > s/succ/success/ ?
Did s/succ/successful/
> On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote:
> > src/zookeeper/detector.cpp, lines 112-149
> > <https://reviews.apache.org/r/13086/diff/6/?file=364283#file364283line112>
> >
> > How about the following?
> >
> > Option<Group::Membership> current;
> >
> > foreach(const Group::Membership& membership, memberships.get()) {
> > if (current.isNone() || membership.id() < current.get().id() {
> > current = membership;
> > }
> > }
> >
> > if (current.iSome() && (!leader.Some() || current.get() != leader.get()) {
> > // Set all promises to current.
> > } else if (current.isNone() && !leader.isNone()) {
> > // Set all promises to None.
> > }
> >
> > promises.clear();
> > leader = current;
> >
> > watch(memberships.get());
> >
> >
> On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote:
> > src/zookeeper/contender.cpp, line 134
> > <https://reviews.apache.org/r/13086/diff/6/?file=364281#file364281line134>
> >
> > Is this supposed to be "&&" or "||" ?
> >
> > What if we started contending but joined() hasn't been called yet? Is that a failure?
> On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote:
> > src/zookeeper/contender.cpp, line 179
> > <https://reviews.apache.org/r/13086/diff/6/?file=364281#file364281line179>
> >
> > How come you are not checking if "success" is ready/failed/discarded?
Because we are funneling the Future, regardless ready/failed/discarded, to the caller waiting on the Future returned by withdraw, which has the same signature.
> On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote:
> > src/zookeeper/contender.cpp, line 195
> > <https://reviews.apache.org/r/13086/diff/6/?file=364281#file364281line195>
> >
> > LOG(ERROR) ?
Should it? The code here propagates the error through Future::failed() and is not swallowing the error.
> On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote:
> > src/zookeeper/contender.cpp, lines 204-212
> > <https://reviews.apache.org/r/13086/diff/6/?file=364281#file364281line204>
> >
> > Can we do this inside cancelled() ?
Removed this because we now fail withdraws when contending is ongoing instead of promising that it will be withdrawn after the membership is obtained.
> On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote:
> > src/zookeeper/contender.cpp, lines 252-255
> > <https://reviews.apache.org/r/13086/diff/6/?file=364281#file364281line252>
> >
> > Again. checking the future is pending here is weird. What happens if it goes out of pending just after this check?
> >
> > Also this should be after the CHECK(memberships.isReady());
Removed this check.
> On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote:
> > src/zookeeper/detector.cpp, line 79
> > <https://reviews.apache.org/r/13086/diff/6/?file=364283#file364283line79>
> >
> > Why not:
> > if (leader.isSome() and !previous.isSome()) {
> > return leader;
> > }
Changed to
if (leader.isSome() && (
!previous.isSome() || leader.get().id() != previous.get().id())) {
return leader;
}
> On Oct. 15, 2013, 7:31 p.m., Vinod Kone wrote:
> > src/zookeeper/group.cpp, lines 865-868
> > <https://reviews.apache.org/r/13086/diff/6/?file=364285#file364285line865>
> >
> > How come we don't do this in expired() and only here?
The current semantics is that when the session expires we transparently clean up things and reconnect and Group's clients will start receiving notifications again. The memberships that this Group "owned" are lost but the benefit is that for watching un-owned memberships the expiration could be hidden from the client.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/#review26995
-----------------------------------------------------------
On Oct. 14, 2013, 11:30 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13086/
> -----------------------------------------------------------
>
> (Updated Oct. 14, 2013, 11:30 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-496
> https://issues.apache.org/jira/browse/MESOS-496
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am a2d82425f74dcaf3adf28ae8184fff53b3c34ceb
> src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
> src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
> src/zookeeper/contender.hpp PRE-CREATION
> src/zookeeper/contender.cpp PRE-CREATION
> src/zookeeper/detector.hpp PRE-CREATION
> src/zookeeper/detector.cpp PRE-CREATION
> src/zookeeper/group.hpp 6b6c1557658df1bb7b7df3ded5c203e2dc0c8308
> src/zookeeper/group.cpp cd58d230a2715b9ed01002443ebd7b5366d31be6
>
> Diff: https://reviews.apache.org/r/13086/diff/
>
>
> Testing
> -------
>
> make check 100 times.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/#review26995
-----------------------------------------------------------
I like the detector code. Its pretty simple and straightforward. We should think more about the contender because currently its a bit complex to follow.
src/tests/zookeeper_tests.cpp
<https://reviews.apache.org/r/13086/#comment52567>
this should come before stout headers.
src/zookeeper/contender.hpp
<https://reviews.apache.org/r/13086/#comment52568>
new line.
src/zookeeper/contender.hpp
<https://reviews.apache.org/r/13086/#comment52569>
s/detector/contender/
src/zookeeper/contender.hpp
<https://reviews.apache.org/r/13086/#comment52573>
I commented about these in the other review.
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment52574>
s/detector/contender/
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment52579>
what is the difference?
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment52575>
how do you know you failed?
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment52576>
s/abort/cancel/?
or
s/abort/fail/ ?
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment52577>
Comments on what each of these represent would be great!
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment52578>
Put initializer on the next line?
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment52643>
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment52626>
s/Cannot contend because//
Considering the caller of this method will/can do
"Failed to contend: " << future.failure()
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment52580>
s/we've/we are/
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment52581>
Not really "only once" right? IIUC, a contend()--> withdraw() --> contend() is valid?
How about? "Already contending. Withdraw first before re-contending" ?
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment52640>
onAny on new line?
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment52628>
Is this supposed to be "&&" or "||" ?
What if we started contending but joined() hasn't been called yet? Is that a failure?
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment52627>
s/Cannot withdraw because we/We/ ?
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment52632>
So, if we are already withdrawing we just return the future. Why not do the same when we are contending?
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment52636>
Not clear what this comment means. Can you clarify?
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment52633>
s/defending// ?
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment52630>
s/succ/success/ ?
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment52634>
How come you are not checking if "success" is ready/failed/discarded?
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment52631>
Do we need the if check? Isn't set going to be a no-op if its already ready/failed?
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment52635>
LOG(ERROR) ?
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment52638>
Can we do this inside cancelled() ?
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment52637>
It seems weird to do it here! Can this be a onAny() callback on contending?
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment52641>
Again. checking the future is pending here is weird. What happens if it goes out of pending just after this check?
Also this should be after the CHECK(memberships.isReady());
src/zookeeper/contender.cpp
<https://reviews.apache.org/r/13086/#comment52642>
A log message here would be nice?
src/zookeeper/detector.hpp
<https://reviews.apache.org/r/13086/#comment52644>
s/membership/leader/
src/zookeeper/detector.hpp
<https://reviews.apache.org/r/13086/#comment52645>
s/specified/previous/
src/zookeeper/detector.cpp
<https://reviews.apache.org/r/13086/#comment52646>
s/has/have/
src/zookeeper/detector.cpp
<https://reviews.apache.org/r/13086/#comment52647>
Why "<" instead of "!="?
src/zookeeper/detector.cpp
<https://reviews.apache.org/r/13086/#comment52648>
Why not:
if (leader.isSome() and !previous.isSome()) {
return leader;
}
src/zookeeper/detector.cpp
<https://reviews.apache.org/r/13086/#comment52649>
A log message here would be nice.
src/zookeeper/detector.cpp
<https://reviews.apache.org/r/13086/#comment52652>
How about the following?
Option<Group::Membership> current;
foreach(const Group::Membership& membership, memberships.get()) {
if (current.isNone() || membership.id() < current.get().id() {
current = membership;
}
}
if (current.iSome() && (!leader.Some() || current.get() != leader.get()) {
// Set all promises to current.
} else if (current.isNone() && !leader.isNone()) {
// Set all promises to None.
}
promises.clear();
leader = current;
watch(memberships.get());
src/zookeeper/detector.cpp
<https://reviews.apache.org/r/13086/#comment52650>
s/the//
src/zookeeper/group.cpp
<https://reviews.apache.org/r/13086/#comment52653>
put out on the previous line.
src/zookeeper/group.cpp
<https://reviews.apache.org/r/13086/#comment52655>
How come we don't do this in expired() and only here?
src/zookeeper/group.cpp
<https://reviews.apache.org/r/13086/#comment52654>
s/expires/expire/
- Vinod Kone
On Oct. 14, 2013, 11:30 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13086/
> -----------------------------------------------------------
>
> (Updated Oct. 14, 2013, 11:30 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-496
> https://issues.apache.org/jira/browse/MESOS-496
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/Makefile.am a2d82425f74dcaf3adf28ae8184fff53b3c34ceb
> src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
> src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
> src/zookeeper/contender.hpp PRE-CREATION
> src/zookeeper/contender.cpp PRE-CREATION
> src/zookeeper/detector.hpp PRE-CREATION
> src/zookeeper/detector.cpp PRE-CREATION
> src/zookeeper/group.hpp 6b6c1557658df1bb7b7df3ded5c203e2dc0c8308
> src/zookeeper/group.cpp cd58d230a2715b9ed01002443ebd7b5366d31be6
>
> Diff: https://reviews.apache.org/r/13086/diff/
>
>
> Testing
> -------
>
> make check 100 times.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/
-----------------------------------------------------------
(Updated Oct. 14, 2013, 11:30 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, and Vinod Kone.
Bugs: MESOS-496
https://issues.apache.org/jira/browse/MESOS-496
Repository: mesos-git
Description
-------
See summary.
Diffs
-----
src/Makefile.am a2d82425f74dcaf3adf28ae8184fff53b3c34ceb
src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
src/zookeeper/contender.hpp PRE-CREATION
src/zookeeper/contender.cpp PRE-CREATION
src/zookeeper/detector.hpp PRE-CREATION
src/zookeeper/detector.cpp PRE-CREATION
src/zookeeper/group.hpp 6b6c1557658df1bb7b7df3ded5c203e2dc0c8308
src/zookeeper/group.cpp cd58d230a2715b9ed01002443ebd7b5366d31be6
Diff: https://reviews.apache.org/r/13086/diff/
Testing
-------
make check 100 times.
Thanks,
Jiang Yan Xu
Re: Review Request 13086: Added Zookeeper leader contender and detector
abstractions.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13086/
-----------------------------------------------------------
(Updated Oct. 14, 2013, 6:54 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Ian Downes, Jie Yu, and Vinod Kone.
Changes
-------
Rebased with master.
Bugs: MESOS-496
https://issues.apache.org/jira/browse/MESOS-496
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
src/Makefile.am a2d82425f74dcaf3adf28ae8184fff53b3c34ceb
src/tests/zookeeper_test_server.cpp 0b22f319a53d08f9fe15c97c634b03358f40ba7e
src/tests/zookeeper_tests.cpp 16c5fb7daaf684ece5542b42dd9e9ec66f4a41ce
src/zookeeper/contender.hpp PRE-CREATION
src/zookeeper/contender.cpp PRE-CREATION
src/zookeeper/detector.hpp PRE-CREATION
src/zookeeper/detector.cpp PRE-CREATION
src/zookeeper/group.hpp 6b6c1557658df1bb7b7df3ded5c203e2dc0c8308
src/zookeeper/group.cpp cd58d230a2715b9ed01002443ebd7b5366d31be6
Diff: https://reviews.apache.org/r/13086/diff/
Testing
-------
make check 100 times.
Thanks,
Jiang Yan Xu