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/11/05 02:09:31 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 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 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 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 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>.
-----------------------------------------------------------
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