mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vinod Kone" <vinodk...@gmail.com>
Subject Re: Review Request 13086: Added Zookeeper leader contender and detector abstractions.
Date Tue, 15 Oct 2013 19:31:57 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message