ONOS-1798: More appropriate method name + better pre-condition checks

Change-Id: I5d277967c22b9db48097288ea072cbc4d1c5a2a8

ONOS-1798: More appropriate method name + better pre-condition checks
Change-Id: I5d277967c22b9db48097288ea072cbc4d1c5a2a8
Madan Jampani · Gerrit Code Review
Commit 9bd1f15b03411f0758869fe8de68a0e54e52faa2 9bd1f15b 1 parent 0fa4bc41
Showing 3 changed files with 19 additions and 29 deletions
core/api/src/main/java/org/onosproject/cluster/LeadershipService.java
core/store/dist/src/main/java/org/onosproject/store/consistent/impl/DistributedLeadershipManager.java
core/store/dist/src/main/java/org/onosproject/store/mastership/impl/ConsistentDeviceMastershipStore.java
--- a/core/api/src/main/java/org/onosproject/cluster/LeadershipService.java
View file @9bd1f15
+++ b/core/api/src/main/java/org/onosproject/cluster/LeadershipService.java
View file @9bd1f15
@@ -74,7 +74,8 @@ public interface LeadershipService {
      * potentially become the leader again if and when it becomes the highest
      * priority candidate
      * <p>
-     * If the local nodeId is not the leader, this method will be a noop.
+     * If the local nodeId is not the leader, this method will make no changes and
+     * simply return false.
      *
      * @param path topic for which this controller node should give up leadership
      * @return true if this node stepped down from leadership, false otherwise
--- a/core/store/dist/src/main/java/org/onosproject/store/consistent/impl/DistributedLeadershipManager.java
View file @9bd1f15
+++ b/core/store/dist/src/main/java/org/onosproject/store/consistent/impl/DistributedLeadershipManager.java
View file @9bd1f15
@@ -271,7 +271,7 @@ public class DistributedLeadershipManager implements LeadershipService {
     @Override
     public boolean stepdown(String path) {
-        if (!activeTopics.contains(path)) {
+        if (!activeTopics.contains(path) || !Objects.equals(localNodeId, getLeader(path))) {
             return false;
         }
--- a/core/store/dist/src/main/java/org/onosproject/store/mastership/impl/ConsistentDeviceMastershipStore.java
View file @9bd1f15
+++ b/core/store/dist/src/main/java/org/onosproject/store/mastership/impl/ConsistentDeviceMastershipStore.java
View file @9bd1f15
@@ -93,7 +93,7 @@ public class ConsistentDeviceMastershipStore
             new MessageSubject("mastership-store-device-role-query");
     private static final MessageSubject ROLE_RELINQUISH_SUBJECT =
             new MessageSubject("mastership-store-device-role-relinquish");
-    private static final MessageSubject MASTERSHIP_RELINQUISH_SUBJECT =
+    private static final MessageSubject TRANSITION_FROM_MASTER_TO_STANDBY_SUBJECT =
             new MessageSubject("mastership-store-device-mastership-relinquish");
     private static final Pattern DEVICE_MASTERSHIP_TOPIC_PATTERN =
@@ -128,9 +128,9 @@ public class ConsistentDeviceMastershipStore
         clusterCommunicator.addSubscriber(ROLE_RELINQUISH_SUBJECT,
                new RoleRelinquishHandler(),
                messageHandlingExecutor);
-        clusterCommunicator.addSubscriber(MASTERSHIP_RELINQUISH_SUBJECT,
+        clusterCommunicator.addSubscriber(TRANSITION_FROM_MASTER_TO_STANDBY_SUBJECT,
                 SERIALIZER::decode,
-                this::relinquishMastership,
+                this::transitionFromMasterToStandby,
                 SERIALIZER::encode,
                 messageHandlingExecutor);
         localNodeId = clusterService.getLocalNode().id();
@@ -143,7 +143,7 @@ public class ConsistentDeviceMastershipStore
     public void deactivate() {
         clusterCommunicator.removeSubscriber(ROLE_QUERY_SUBJECT);
         clusterCommunicator.removeSubscriber(ROLE_RELINQUISH_SUBJECT);
-        clusterCommunicator.removeSubscriber(MASTERSHIP_RELINQUISH_SUBJECT);
+        clusterCommunicator.removeSubscriber(TRANSITION_FROM_MASTER_TO_STANDBY_SUBJECT);
         messageHandlingExecutor.shutdown();
         leadershipService.removeListener(leadershipEventListener);
@@ -256,7 +256,7 @@ public class ConsistentDeviceMastershipStore
                 return null;
             }
             if (leadershipService.makeTopCandidate(leadershipTopic, nodeId)) {
-                return relinquishMastership(deviceId);
+                return transitionFromMasterToStandby(deviceId);
             } else {
                 log.warn("Failed to promote {} to mastership for {}", nodeId, deviceId);
             }
@@ -282,9 +282,9 @@ public class ConsistentDeviceMastershipStore
         if (!nodeId.equals(currentMaster)) {
             return null;
         }
-        // FIXME: This can becomes the master again unless it
+        // FIXME: This can become the master again unless it
-        // is demoted to the end of candidates list.
+        // is first demoted to the end of candidates list.
-        return relinquishMastership(deviceId);
+        return transitionFromMasterToStandby(deviceId);
     }
     @Override
@@ -309,14 +309,11 @@ public class ConsistentDeviceMastershipStore
         }
         String leadershipTopic = createDeviceMastershipTopic(deviceId);
-        Leadership currentLeadership = leadershipService.getLeadership(leadershipTopic);
+        NodeId currentLeader = leadershipService.getLeader(leadershipTopic);
-        MastershipEvent.Type eventType = null;
+        MastershipEvent.Type eventType = Objects.equal(currentLeader, localNodeId)
-        if (currentLeadership != null && currentLeadership.leader().equals(localNodeId)) {
+            ? MastershipEvent.Type.MASTER_CHANGED
-            eventType = MastershipEvent.Type.MASTER_CHANGED;
+            : MastershipEvent.Type.BACKUPS_CHANGED;
-        } else {
-            eventType = MastershipEvent.Type.BACKUPS_CHANGED;
-        }
         connectedDevices.remove(deviceId);
         leadershipService.withdraw(leadershipTopic);
@@ -324,7 +321,7 @@ public class ConsistentDeviceMastershipStore
         return new MastershipEvent(eventType, deviceId, getNodes(deviceId));
     }
-    private MastershipEvent relinquishMastership(DeviceId deviceId) {
+    private MastershipEvent transitionFromMasterToStandby(DeviceId deviceId) {
         checkArgument(deviceId != null, DEVICE_ID_NULL);
         NodeId currentMaster = getMaster(deviceId);
@@ -337,22 +334,14 @@ public class ConsistentDeviceMastershipStore
                     + "mastership for device {} to {}", deviceId, currentMaster);
             return futureGetOrElse(clusterCommunicator.sendAndReceive(
                     deviceId,
-                    MASTERSHIP_RELINQUISH_SUBJECT,
+                    TRANSITION_FROM_MASTER_TO_STANDBY_SUBJECT,
                     SERIALIZER::encode,
                     SERIALIZER::decode,
                     currentMaster), null);
         }
-        String leadershipTopic = createDeviceMastershipTopic(deviceId);
+        return leadershipService.stepdown(createDeviceMastershipTopic(deviceId))
-        Leadership currentLeadership = leadershipService.getLeadership(leadershipTopic);
+                ? new MastershipEvent(MastershipEvent.Type.MASTER_CHANGED, deviceId, getNodes(deviceId)) : null;
-
-        MastershipEvent.Type eventType = null;
-        if (currentLeadership != null && currentLeadership.leader().equals(localNodeId)) {
-            eventType = MastershipEvent.Type.MASTER_CHANGED;
-        }
-
-        return leadershipService.stepdown(leadershipTopic)
-                ? new MastershipEvent(eventType, deviceId, getNodes(deviceId)) : null;
     }
     private class RoleQueryHandler implements ClusterMessageHandler {