Mailing-List: contact mesos-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: mesos-dev@incubator.apache.org
Content-Type: multipart/alternative;
	boundary="===============5810030408958975304=="
MIME-Version: 1.0
Subject: Re: Review Request: Added a ZooKeeper leader detector and leader
 contender
 abstraction (and a few small changes done while accomplishing this).
From: "Benjamin Hindman" <benh@berkeley.edu>
To: "John Sirois" <john.sirois@gmail.com>
Date: Thu, 16 Feb 2012 22:47:49 -0000
Message-ID: <20120216224749.7047.97544@reviews.apache.org>
Cc: "Benjamin Hindman" <benh@berkeley.edu>,
	"mesos" <mesos-dev@incubator.apache.org>
In-Reply-To: <20120216014721.18863.86173@reviews.apache.org>
References: <20120216014721.18863.86173@reviews.apache.org>

--===============5810030408958975304==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3923/#review5169
-----------------------------------------------------------


src/common/logging.hpp
<https://reviews.apache.org/r/3923/#comment11322>

    Isn't that what my comment says? ;)


src/log/network.hpp
<https://reviews.apache.org/r/3923/#comment11323>

    I just copied the code that we've been using for a while. To be clear, =
all recoverable/retryable ZK errors do not propagate here, they are handled=
 under the covers in Group. An error here really means: you can not use you=
r instance of Group any more and you can not talk to ZK. Alternatives for L=
OG(FATAL) include (1) just logging and never updating the set of pids again=
 in the future or (2) trying to create a new Group instance. I'm more keen =
about (2) than (1). I'm not a fan of (1) because that could be a gnarly thi=
ng to debug (I'd rather have the process get killed at this point) and my c=
oncern about (2) is that we could get into an infinite loop constantly retr=
ying. =


src/zookeeper/contender.hpp
<https://reviews.apache.org/r/3923/#comment11328>

    Adding more discussion around semantics of calling contend.


src/zookeeper/contender.hpp
<https://reviews.apache.org/r/3923/#comment11329>

    Updated comment.


src/zookeeper/contender.hpp
<https://reviews.apache.org/r/3923/#comment11330>

    Yup.


src/zookeeper/contender.cpp
<https://reviews.apache.org/r/3923/#comment11327>

    I'm trying to be explicit, so future people don't need to make assumpti=
ons about why I left out conditionals.


src/zookeeper/contender.cpp
<https://reviews.apache.org/r/3923/#comment11325>

    In this case an election has occurred where this contender won and they=
 haven't found out about it. So, it's debatable whether or not I should ign=
ore that election. That being said, I think I'm still missing some cases. U=
pdated review forthcoming.


src/zookeeper/contender.cpp
<https://reviews.apache.org/r/3923/#comment11324>

    Chatted with Bill, this is better: CHECK(contending || !elected);


src/zookeeper/detector.cpp
<https://reviews.apache.org/r/3923/#comment11326>

    So all I have is Group::Membership's here, no data associated with thos=
e memberships. I could add that lookup for the sake of logging, but that se=
ems a bit overkill. Instead I'm actually going to kill this here and re-add=
 these lines in the MasterDetector/MasterContender version that is libproce=
ss/UPID specific.


- Benjamin


On 2012-02-16 01:47:21, Benjamin Hindman wrote:
> =

> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3923/
> -----------------------------------------------------------
> =

> (Updated 2012-02-16 01:47:21)
> =

> =

> Review request for mesos and John Sirois.
> =

> =

> Summary
> -------
> =

> See summary.
> =

> =

> Diffs
> -----
> =

>   src/Makefile.am c58e9d8 =

>   src/common/logging.hpp f0cee46 =

>   src/log/network.hpp 0f7fefd =

>   src/tests/zookeeper_tests.cpp a852103 =

>   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 695a8ab =

>   src/zookeeper/group.cpp c9122f6 =

> =

> Diff: https://reviews.apache.org/r/3923/diff
> =

> =

> Testing
> -------
> =

> make && make check
> =

> =

> Thanks,
> =

> Benjamin
> =

>


--===============5810030408958975304==--