[4/5] git commit: LeaderContender::withdraw() now always returns false if there is no membership to withdraw.

[vi...@apache.org]

LeaderContender::withdraw() now always returns false if there is no
membership to withdraw.

This includes when:
- withdraw() is called before contend()
- withdraw() is called after contend() but before the contender learns that it fails to obtain the candidacy
- withdraw() is called after the contender has learned that it had failed to obtain the candidacy

From: Jiang Yan Xu <ya...@jxu.me>
Review: https://reviews.apache.org/r/16590


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/682b0d99
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/682b0d99
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/682b0d99

Branch: refs/heads/master
Commit: 682b0d9917960fd4f976eb26dc5fdb1d9957c692
Parents: b2516db
Author: Vinod Kone <vi...@twitter.com>
Authored: Wed Jan 15 12:51:49 2014 -0800
Committer: Vinod Kone <vi...@twitter.com>
Committed: Wed Jan 15 12:54:58 2014 -0800

----------------------------------------------------------------------
 src/tests/zookeeper_tests.cpp |  9 ++++++-
 src/zookeeper/contender.cpp   | 50 ++++++++++++++++++--------------------
 src/zookeeper/contender.hpp   |  4 +--
 3 files changed, 33 insertions(+), 30 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/682b0d99/src/tests/zookeeper_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/zookeeper_tests.cpp b/src/tests/zookeeper_tests.cpp
index 94d324a..656afde 100644
--- a/src/tests/zookeeper_tests.cpp
+++ b/src/tests/zookeeper_tests.cpp
@@ -267,11 +267,18 @@ TEST_F(ZooKeeperTest, LeaderContender)
 
   Owned<LeaderContender> contender(
       new LeaderContender(&group, "candidate 1"));
+
+  // Calling withdraw before contending returns 'false' because there
+  // is nothing to withdraw.
+  Future<bool> withdrawn = contender->withdraw();
+  AWAIT_READY(withdrawn);
+  EXPECT_FALSE(withdrawn.get());
+
   contender->contend();
 
   // Immediately withdrawing after contending leads to delayed
   // cancellation.
-  Future<bool> withdrawn = contender->withdraw();
+  withdrawn = contender->withdraw();
   AWAIT_READY(withdrawn);
   EXPECT_TRUE(withdrawn.get());
 

http://git-wip-us.apache.org/repos/asf/mesos/blob/682b0d99/src/zookeeper/contender.cpp
----------------------------------------------------------------------
diff --git a/src/zookeeper/contender.cpp b/src/zookeeper/contender.cpp
index e857afa..d8a5201 100644
--- a/src/zookeeper/contender.cpp
+++ b/src/zookeeper/contender.cpp
@@ -40,9 +40,6 @@ private:
   // Invoked when the group membership is cancelled.
   void cancelled(const Future<bool>& result);
 
-  // Helper for setting error and failing pending promises.
-  void fail(const string& message);
-
   // Helper for cancelling the Group membership.
   void cancel();
 
@@ -104,8 +101,8 @@ void LeaderContenderProcess::finalize()
 {
   // We do not wait for the result here because the Group keeps
   // retrying (even after the contender is destroyed) until it
-  // either succeeds or its session times out. In either case the
-  // old membership is eventually cancelled.
+  // succeeds so the old membership is eventually going to be
+  // cancelled.
   // There is a tricky situation where the contender terminates after
   // it has contended but before it is notified of the obtained
   // membership. In this case the membership is not cancelled during
@@ -135,7 +132,8 @@ Future<Future<Nothing> > LeaderContenderProcess::contend()
 Future<bool> LeaderContenderProcess::withdraw()
 {
   if (contending.isNone()) {
-    return Failure("Can only withdraw after the contender has contended");
+    // Nothing to withdraw because the contender has not contended.
+    return false;
   }
 
   if (withdrawing.isSome()) {
@@ -169,6 +167,9 @@ void LeaderContenderProcess::cancel()
 {
   if (!candidacy.isReady()) {
     // Nothing to cancel.
+    if (withdrawing.isSome()) {
+      withdrawing.get()->set(false);
+    }
     return;
   }
 
@@ -191,7 +192,13 @@ void LeaderContenderProcess::cancelled(const Future<bool>& result)
   CHECK(!result.isDiscarded());
 
   if (result.isFailed()) {
-    fail(result.failure());
+    if (withdrawing.isSome()) {
+      withdrawing.get()->fail(result.failure());
+    }
+
+    if (watching.isSome()) {
+      watching.get()->fail(result.failure());
+    }
   } else {
     if (withdrawing.isSome()) {
       withdrawing.get()->set(result);
@@ -208,13 +215,22 @@ void LeaderContenderProcess::joined()
 {
   CHECK(!candidacy.isDiscarded());
 
+  // Cannot be watching because the candidacy is not obtained yet.
+  CHECK(watching.isNone());
+
+  CHECK_SOME(contending);
+
   if (candidacy.isFailed()) {
-    fail(candidacy.failure());
+    // The promise 'withdrawing' will be set to false in cancel().
+    contending.get()->fail(candidacy.failure());
     return;
   }
 
   if (withdrawing.isSome()) {
     LOG(INFO) << "Joined group after the contender started withdrawing";
+
+    // The promise 'withdrawing' will be set to 'false' in subsequent
+    // 'cancel()' call.
     return;
   }
 
@@ -222,11 +238,9 @@ void LeaderContenderProcess::joined()
             << data << "') has entered the contest for leadership";
 
   // Transition to 'watching' state.
-  CHECK(watching.isNone());
   watching = new Promise<Nothing>();
 
   // Notify the client.
-  CHECK_SOME(contending);
   if (contending.get()->set(watching.get()->future())) {
     // Continue to watch that our membership is not removed (if the
     // client still cares about it).
@@ -236,22 +250,6 @@ void LeaderContenderProcess::joined()
 }
 
 
-void LeaderContenderProcess::fail(const string& message)
-{
-  if (contending.isSome()) {
-    contending.get()->fail(message);
-  }
-
-  if (watching.isSome()) {
-    watching.get()->fail(message);
-  }
-
-  if (withdrawing.isSome()) {
-    withdrawing.get()->fail(message);
-  }
-}
-
-
 LeaderContender::LeaderContender(Group* group, const string& data)
 {
   process = new LeaderContenderProcess(group, data);

http://git-wip-us.apache.org/repos/asf/mesos/blob/682b0d99/src/zookeeper/contender.hpp
----------------------------------------------------------------------
diff --git a/src/zookeeper/contender.hpp b/src/zookeeper/contender.hpp
index f45c0df..e526e2d 100644
--- a/src/zookeeper/contender.hpp
+++ b/src/zookeeper/contender.hpp
@@ -45,11 +45,9 @@ public:
   // Returns true if successfully withdrawn from the contest (either
   // while contending or has already contended and is watching for
   // membership loss).
-  // It should only be called after contend() is called, otherwise a
-  // failure is returned.
   // 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.
+  // an expired membership or because there is nothing to withdraw.
   // A failed future is returned if the contender is unable to
   // withdraw.
   process::Future<bool> withdraw();