QPID-5715: Address review comments from Alex Rudyy

git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk@1591366 13f79535-47bb-0310-9956-ffa450edef68

2 files changed, 54 insertions, 23 deletions

diff --git a/qpid/java/bdbstore/src/main/java/org/apache/qpid/server/virtualhostnode/berkeleydb/BDBHAVirtualHostNodeImpl.java b/qpid/java/bdbstore/src/main/java/org/apache/qpid/server/virtualhostnode/berkeleydb/BDBHAVirtualHostNodeImpl.java
index 1f647652c2..95f542585c 100644
--- a/qpid/java/bdbstore/src/main/java/org/apache/qpid/server/virtualhostnode/berkeleydb/BDBHAVirtualHostNodeImpl.java
+++ b/qpid/java/bdbstore/src/main/java/org/apache/qpid/server/virtualhostnode/berkeleydb/BDBHAVirtualHostNodeImpl.java
@@ -24,6 +24,7 @@ import java.security.PrivilegedAction;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
@@ -37,8 +38,8 @@ import com.sleepycat.je.rep.ReplicationNode;
 import com.sleepycat.je.rep.StateChangeEvent;
 import com.sleepycat.je.rep.StateChangeListener;
 
-import org.apache.qpid.server.configuration.IllegalConfigurationException;
 import org.apache.log4j.Logger;
+import org.apache.qpid.server.configuration.IllegalConfigurationException;
 import org.apache.qpid.server.logging.messages.ConfigStoreMessages;
 import org.apache.qpid.server.model.Broker;
 import org.apache.qpid.server.model.BrokerModel;
@@ -107,7 +108,7 @@ public class BDBHAVirtualHostNodeImpl extends AbstractVirtualHostNode<BDBHAVirtu
     @ManagedAttributeField(afterSet="postSetQuorumOverride")
     private int _quorumOverride;
 
-    @ManagedAttributeField(beforeSet="preSetRole", afterSet="postSetRole")
+    @ManagedAttributeField(afterSet="postSetRole")
     private String _role;
 
     @ManagedAttributeField
@@ -121,6 +122,25 @@ public class BDBHAVirtualHostNodeImpl extends AbstractVirtualHostNode<BDBHAVirtu
     }
 
     @Override
+    protected void validateChange(final ConfiguredObject<?> proxyForValidation, final Set<String> changedAttributes)
+    {
+        super.validateChange(proxyForValidation, changedAttributes);
+        if (changedAttributes.contains(ROLE))
+        {
+            String currentRole = getRole();
+            if (!ReplicatedEnvironment.State.REPLICA.name().equals(currentRole))
+            {
+                throw new IllegalStateException("Cannot transfer mastership when not a replica, current role is " + currentRole);
+            }
+            BDBHAVirtualHostNode<?> proposed = (BDBHAVirtualHostNode<?>)proxyForValidation;
+            if (!ReplicatedEnvironment.State.MASTER.name().equals(proposed.getRole()))
+            {
+                throw new IllegalArgumentException("Changing role to other value then " + ReplicatedEnvironment.State.MASTER.name() + " is unsupported");
+            }
+        }
+    }
+
+    @Override
     public Map<String, String> getEnvironmentConfiguration()
     {
         return _environmentConfiguration;
@@ -530,26 +550,6 @@ public class BDBHAVirtualHostNodeImpl extends AbstractVirtualHostNode<BDBHAVirtu
         }
     }
 
-    // used as pre action by field _role
-    @SuppressWarnings("unused")
-    private void preSetRole()
-    {
-        ReplicatedEnvironmentFacade environmentFacade = getReplicatedEnvironmentFacade();
-        if (environmentFacade != null)
-        {
-            String currentRole = environmentFacade.getNodeState();
-            if (!ReplicatedEnvironment.State.REPLICA.name().equals(currentRole))
-            {
-                 throw new IllegalConfigurationException("Cannot transfer mastership when node is not in a replica role."
-                         + "Current role is " + currentRole);
-             }
-        }
-        else
-        {
-            // Ignored
-        }
-    }
-
     // used as post action by field _role
     @SuppressWarnings("unused")
     private void postSetRole()
diff --git a/qpid/java/bdbstore/src/test/java/org/apache/qpid/server/store/berkeleydb/BDBHAVirtualHostNodeTest.java b/qpid/java/bdbstore/src/test/java/org/apache/qpid/server/store/berkeleydb/BDBHAVirtualHostNodeTest.java
index 826694904c..78a86c685d 100644
--- a/qpid/java/bdbstore/src/test/java/org/apache/qpid/server/store/berkeleydb/BDBHAVirtualHostNodeTest.java
+++ b/qpid/java/bdbstore/src/test/java/org/apache/qpid/server/store/berkeleydb/BDBHAVirtualHostNodeTest.java
@@ -246,7 +246,6 @@ public class BDBHAVirtualHostNodeTest extends QpidTestCase
 
         assertNotNull("Join time should be set", node.getJoinTime());
         assertNotNull("Last known replication transaction idshould be set", node.getLastKnownReplicationTransactionId());
-
     }
 
     public void testTransferMasterToSelf() throws Exception
@@ -421,6 +420,38 @@ public class BDBHAVirtualHostNodeTest extends QpidTestCase
         }
     }
 
+    public void testMutatingRoleWhenNotReplica_IsDisallowed() throws Exception
+    {
+        int nodePortNumber = findFreePort();
+        String helperAddress = "localhost:" + nodePortNumber;
+        String groupName = "group";
+
+        Map<String, Object> node1Attributes = new HashMap<String, Object>();
+        node1Attributes.put(BDBHAVirtualHostNode.ID, UUID.randomUUID());
+        node1Attributes.put(BDBHAVirtualHostNode.TYPE, "BDB_HA");
+        node1Attributes.put(BDBHAVirtualHostNode.NAME, "node1");
+        node1Attributes.put(BDBHAVirtualHostNode.GROUP_NAME, groupName);
+        node1Attributes.put(BDBHAVirtualHostNode.ADDRESS, helperAddress);
+        node1Attributes.put(BDBHAVirtualHostNode.HELPER_ADDRESS, helperAddress);
+        node1Attributes.put(BDBHAVirtualHostNode.STORE_PATH, _bdbStorePath + File.separator + "1");
+
+        BDBHAVirtualHostNode<?> node = createHaVHN(node1Attributes);
+        assertEquals("Failed to activate node", State.ACTIVE, node.setDesiredState(node.getState(), State.ACTIVE));
+
+        assertEquals("Node is expected to be master", "MASTER", node.getRole());
+
+        try
+        {
+            node.setAttributes(Collections.<String,Object>singletonMap(BDBHAVirtualHostNode.ROLE, "REPLICA"));
+            fail("Role mutation should fail");
+        }
+        catch(IllegalStateException e)
+        {
+            // PASS
+        }
+    }
+
+
     private BDBHAVirtualHostNode<?> createHaVHN(Map<String, Object> attributes)
     {
         BDBHAVirtualHostNode<?> node = (BDBHAVirtualHostNode<?>) _objectFactory.create(VirtualHostNode.class, attributes, _broker);