Addressed Checkstyle and PMD violations in the registry module.
Note some PMD errors can't be cleaned up because the curator API throws
raw Exception on many methods. PMD complains about catching raw Exception.
Change-Id: I974e3e713417d71de8fcee3833d53ab003a71eae
diff --git a/src/main/java/net/floodlightcontroller/core/module/FloodlightModuleException.java b/src/main/java/net/floodlightcontroller/core/module/FloodlightModuleException.java
index 8519a3c..e4232fc 100644
--- a/src/main/java/net/floodlightcontroller/core/module/FloodlightModuleException.java
+++ b/src/main/java/net/floodlightcontroller/core/module/FloodlightModuleException.java
@@ -3,7 +3,11 @@
public class FloodlightModuleException extends Exception {
private static final long serialVersionUID = 1L;
- public FloodlightModuleException(String error) {
- super(error);
+ public FloodlightModuleException(String message) {
+ super(message);
+ }
+
+ public FloodlightModuleException(String message, Throwable cause) {
+ super(message, cause);
}
}
diff --git a/src/main/java/net/onrc/onos/core/registry/ControllerRegistryEntry.java b/src/main/java/net/onrc/onos/core/registry/ControllerRegistryEntry.java
index 569a922..c6d730d 100644
--- a/src/main/java/net/onrc/onos/core/registry/ControllerRegistryEntry.java
+++ b/src/main/java/net/onrc/onos/core/registry/ControllerRegistryEntry.java
@@ -2,15 +2,14 @@
import org.codehaus.jackson.annotate.JsonProperty;
-
public class ControllerRegistryEntry implements Comparable<ControllerRegistryEntry> {
//
// TODO: Refactor the implementation and decide whether controllerId
// is needed. If "yes", we might need to consider it inside the
- // compareTo(), equals() and hashCode() impmenetations.
+ // compareTo(), equals() and hashCode() implementations.
//
- private String controllerId;
- private int sequenceNumber;
+ private final String controllerId;
+ private final int sequenceNumber;
public ControllerRegistryEntry(String controllerId, int sequenceNumber) {
this.controllerId = controllerId;
diff --git a/src/main/java/net/onrc/onos/core/registry/IdBlock.java b/src/main/java/net/onrc/onos/core/registry/IdBlock.java
index d905917..7184239 100644
--- a/src/main/java/net/onrc/onos/core/registry/IdBlock.java
+++ b/src/main/java/net/onrc/onos/core/registry/IdBlock.java
@@ -1,9 +1,9 @@
package net.onrc.onos.core.registry;
public class IdBlock {
- private long start;
- private long end;
- private long size;
+ private final long start;
+ private final long end;
+ private final long size;
public IdBlock(long start, long end, long size) {
this.start = start;
diff --git a/src/main/java/net/onrc/onos/core/registry/StandaloneRegistry.java b/src/main/java/net/onrc/onos/core/registry/StandaloneRegistry.java
index 989d2c1..de07adf 100644
--- a/src/main/java/net/onrc/onos/core/registry/StandaloneRegistry.java
+++ b/src/main/java/net/onrc/onos/core/registry/StandaloneRegistry.java
@@ -15,6 +15,7 @@
import net.floodlightcontroller.restserver.IRestApiService;
import net.onrc.onos.core.registry.web.RegistryWebRoutable;
+import org.apache.commons.lang.NotImplementedException;
import org.openflow.util.HexString;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -23,17 +24,15 @@
* Implementation of a registry that doesn't rely on any external registry
* service. This is designed to be used only in single-node setups (e.g. for
* development). All registry data is stored in local memory.
- *
- * @author jono
*/
public class StandaloneRegistry implements IFloodlightModule,
IControllerRegistryService {
private static final Logger log = LoggerFactory.getLogger(StandaloneRegistry.class);
- protected IRestApiService restApi;
+ private IRestApiService restApi;
- protected String registeredControllerId = null;
- protected Map<String, ControlChangeCallback> switchCallbacks;
+ private String registeredControllerId;
+ private Map<String, ControlChangeCallback> switchCallbacks;
//
// Unique ID generation state
@@ -44,7 +43,7 @@
public void requestControl(long dpid, ControlChangeCallback cb)
throws RegistryException {
if (registeredControllerId == null) {
- throw new RuntimeException(
+ throw new IllegalStateException(
"Must register a controller before calling requestControl");
}
@@ -52,7 +51,7 @@
log.debug("Control granted for {}", HexString.toHexString(dpid));
- //Immediately grant request for control
+ // Immediately grant request for control
if (cb != null) {
cb.controlChanged(dpid, true);
}
@@ -103,7 +102,7 @@
@Override
public String getControllerForSwitch(long dpid) throws RegistryException {
- return (switchCallbacks.get(HexString.toHexString(dpid)) != null) ? registeredControllerId : null;
+ return (switchCallbacks.get(HexString.toHexString(dpid)) == null) ? null : registeredControllerId;
}
@Override
@@ -125,25 +124,29 @@
@Override
public Collection<Long> getSwitchesControlledByController(
String controllerId) {
- throw new RuntimeException("Not yet implemented");
+ throw new NotImplementedException("Not yet implemented");
}
- private long blockTop = 0L;
+ private long blockTop;
private static final long BLOCK_SIZE = 0x1000000L;
/**
* Returns a block of IDs which are unique and unused.
* Range of IDs is fixed size and is assigned incrementally as this method called.
+ *
+ * @return an IdBlock containing a set of unique IDs
*/
@Override
- public synchronized IdBlock allocateUniqueIdBlock() {
- long blockHead = blockTop;
- long blockTail = blockTop + BLOCK_SIZE;
+ public IdBlock allocateUniqueIdBlock() {
+ synchronized (this) {
+ long blockHead = blockTop;
+ long blockTail = blockTop + BLOCK_SIZE;
- IdBlock block = new IdBlock(blockHead, blockTail - 1, BLOCK_SIZE);
- blockTop = blockTail;
+ IdBlock block = new IdBlock(blockHead, blockTail - 1, BLOCK_SIZE);
+ blockTop = blockTail;
- return block;
+ return block;
+ }
}
/**
@@ -196,7 +199,7 @@
@Override
public IdBlock allocateUniqueIdBlock(long range) {
- throw new UnsupportedOperationException("Not supported yet."); //To change body of generated methods, choose Tools | Templates.
+ throw new UnsupportedOperationException("Not supported yet");
}
}
diff --git a/src/main/java/net/onrc/onos/core/registry/ZookeeperRegistry.java b/src/main/java/net/onrc/onos/core/registry/ZookeeperRegistry.java
index 230b39b..2f1c4c7 100644
--- a/src/main/java/net/onrc/onos/core/registry/ZookeeperRegistry.java
+++ b/src/main/java/net/onrc/onos/core/registry/ZookeeperRegistry.java
@@ -23,6 +23,7 @@
import net.floodlightcontroller.restserver.IRestApiService;
import net.onrc.onos.core.registry.web.RegistryWebRoutable;
+import org.apache.commons.lang.NotImplementedException;
import org.apache.curator.RetryPolicy;
import org.apache.curator.framework.CuratorFramework;
import org.apache.curator.framework.CuratorFrameworkFactory;
@@ -57,46 +58,47 @@
public class ZookeeperRegistry implements IFloodlightModule, IControllerRegistryService {
private static final Logger log = LoggerFactory.getLogger(ZookeeperRegistry.class);
- protected String controllerId = null;
- protected IRestApiService restApi;
+ private String controllerId;
+
+ private IRestApiService restApi;
//This is the default, it's overwritten by the connectionString configuration parameter
- protected String connectionString = "localhost:2181";
+ private String connectionString = "localhost:2181";
- private final String namespace = "onos";
- private final String switchLatchesPath = "/switches";
+ private static final String NAMESPACE = "onos";
+ private static final String SWITCH_LATCHES_PATH = "/switches";
private static final String CLUSTER_LEADER_PATH = "/cluster/leader";
private static final String SERVICES_PATH = "/"; //i.e. the root of our namespace
private static final String CONTROLLER_SERVICE_NAME = "controllers";
- protected CuratorFramework curatorFrameworkClient;
+ private CuratorFramework curatorFrameworkClient;
- protected PathChildrenCache rootSwitchCache;
+ private PathChildrenCache rootSwitchCache;
- protected ConcurrentHashMap<String, SwitchLeadershipData> switches;
- protected Map<String, PathChildrenCache> switchPathCaches;
+ private ConcurrentHashMap<String, SwitchLeadershipData> switches;
+ private Map<String, PathChildrenCache> switchPathCaches;
- protected LeaderLatch clusterLeaderLatch;
- protected ClusterLeaderListener clusterLeaderListener;
+ private LeaderLatch clusterLeaderLatch;
+ private ClusterLeaderListener clusterLeaderListener;
private static final long CLUSTER_LEADER_ELECTION_RETRY_MS = 100;
private static final String ID_COUNTER_PATH = "/flowidcounter";
private static final Long ID_BLOCK_SIZE = 0x100000000L;
- protected DistributedAtomicLong distributedIdCounter;
+ private DistributedAtomicLong distributedIdCounter;
//Zookeeper performance-related configuration
- protected static final int SESSION_TIMEOUT = 5000;
- protected static final int CONNECTION_TIMEOUT = 7000;
+ private static final int SESSION_TIMEOUT = 7000; // ms
+ private static final int CONNECTION_TIMEOUT = 5000; // ms
//
// Unique ID generation state
// TODO: The implementation must be updated to use the Zookeeper
- // instead of a ramdon generator.
+ // instead of a random generator.
//
private static Random randomGenerator = new Random();
- private static long nextUniqueIdPrefix = 0;
+ private static long nextUniqueIdPrefix;
// NOTE: The 0xffffffffL value is used by the Unique ID generator for
// initialization purpose.
private static long nextUniqueIdSuffix = 0xffffffffL;
@@ -104,16 +106,33 @@
private final BlockingQueue<SwitchLeaderEvent> switchLeadershipEvents =
new LinkedBlockingQueue<SwitchLeaderEvent>();
- private ExecutorService eventThreadExecutorService;
+ /**
+ * Listens for changes to the switch znodes in Zookeeper. This maintains
+ * the second level of PathChildrenCaches that hold the controllers
+ * contending for each switch - there's one for each switch.
+ */
+ private PathChildrenCacheListener switchPathCacheListener =
+ new SwitchPathCacheListener();
+ private ServiceDiscovery<ControllerService> serviceDiscovery;
+ private ServiceCache<ControllerService> serviceCache;
+
private static class SwitchLeaderEvent {
- public final long dpid;
- public final boolean isLeader;
+ private final long dpid;
+ private final boolean isLeader;
public SwitchLeaderEvent(long dpid, boolean isLeader) {
this.dpid = dpid;
this.isLeader = isLeader;
}
+
+ public long getDpid() {
+ return dpid;
+ }
+
+ public boolean isLeader() {
+ return isLeader;
+ }
}
/*
@@ -123,13 +142,13 @@
while (!Thread.currentThread().isInterrupted()) {
try {
SwitchLeaderEvent event = switchLeadershipEvents.take();
- SwitchLeadershipData swData = switches.get(HexString.toHexString(event.dpid));
+ SwitchLeadershipData swData = switches.get(HexString.toHexString(event.getDpid()));
if (swData == null) {
- log.debug("Leadership data {} not found", event.dpid);
+ log.debug("Leadership data {} not found", event.getDpid());
continue;
}
- swData.getCallback().controlChanged(event.dpid, event.isLeader);
+ swData.getCallback().controlChanged(event.getDpid(), event.isLeader());
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
break;
@@ -139,8 +158,8 @@
}
}
- protected class SwitchLeaderListener implements LeaderLatchListener {
- String dpid;
+ class SwitchLeaderListener implements LeaderLatchListener {
+ private String dpid;
public SwitchLeaderListener(String dpid) {
this.dpid = dpid;
@@ -161,7 +180,7 @@
}
}
- protected class SwitchPathCacheListener implements PathChildrenCacheListener {
+ class SwitchPathCacheListener implements PathChildrenCacheListener {
@Override
public void childEvent(CuratorFramework client,
PathChildrenCacheEvent event) throws Exception {
@@ -204,10 +223,7 @@
}
}
- protected static class ClusterLeaderListener implements LeaderLatchListener {
- public ClusterLeaderListener() {
- }
-
+ private static class ClusterLeaderListener implements LeaderLatchListener {
//
// NOTE: If we need to support callbacks when the
// leadership changes, those should be called here.
@@ -224,32 +240,23 @@
}
}
- /**
- * Listens for changes to the switch znodes in Zookeeper. This maintains
- * the second level of PathChildrenCaches that hold the controllers
- * contending for each switch - there's one for each switch.
- */
- PathChildrenCacheListener switchPathCacheListener = new SwitchPathCacheListener();
- protected ServiceDiscovery<ControllerService> serviceDiscovery;
- protected ServiceCache<ControllerService> serviceCache;
-
-
@Override
public void requestControl(long dpid, ControlChangeCallback cb) throws RegistryException {
log.info("Requesting control for {}", HexString.toHexString(dpid));
if (controllerId == null) {
- throw new RuntimeException("Must register a controller before calling requestControl");
+ throw new IllegalStateException("Must register a controller before calling requestControl");
}
String dpidStr = HexString.toHexString(dpid);
- String latchPath = switchLatchesPath + "/" + dpidStr;
if (switches.get(dpidStr) != null) {
log.debug("Already contesting {}, returning", HexString.toHexString(dpid));
throw new RegistryException("Already contesting control for " + dpidStr);
}
+ String latchPath = SWITCH_LATCHES_PATH + "/" + dpidStr;
+
LeaderLatch latch = new LeaderLatch(curatorFrameworkClient, latchPath, controllerId);
SwitchLeaderListener listener = new SwitchLeaderListener(dpidStr);
latch.addListener(listener);
@@ -356,17 +363,15 @@
controllerId = id;
try {
- ServiceInstance<ControllerService> thisInstance = ServiceInstance.<ControllerService>builder()
+ ServiceInstance<ControllerService> thisInstance =
+ ServiceInstance.<ControllerService>builder()
.name(CONTROLLER_SERVICE_NAME)
.payload(new ControllerService(controllerId))
- //.port((int)(65535 * Math.random())) // in a real application, you'd use a common port
- //.uriSpec(uriSpec)
.build();
serviceDiscovery.registerService(thisInstance);
} catch (Exception e) {
- // TODO Auto-generated catch block
- e.printStackTrace();
+ log.error("Exception starting service instance:", e);
}
}
@@ -389,8 +394,7 @@
//uses this and that isn't particularly time-sensitive.
switchCache.rebuild();
} catch (Exception e) {
- // TODO Auto-generated catch block
- e.printStackTrace();
+ log.error("Exception rebuilding the switch cache:", e);
}
List<ChildData> sortedData = new ArrayList<ChildData>(switchCache.getCurrentData());
@@ -410,7 +414,7 @@
}
);
- if (sortedData.size() == 0) {
+ if (sortedData.isEmpty()) {
return null;
}
@@ -420,7 +424,7 @@
@Override
public Collection<Long> getSwitchesControlledByController(String controller) {
//TODO remove this if not needed
- throw new RuntimeException("Not yet implemented");
+ throw new NotImplementedException("Not yet implemented");
}
@@ -474,9 +478,11 @@
/**
* Returns a block of IDs which are unique and unused.
- * Range of IDs is fixed size and is assigned incrementally as this method called.
- * Since the range of IDs is managed by Zookeeper in distributed way, this method may block when
- * requests come up simultaneously.
+ * The range of IDs is a fixed size and is allocated incrementally as this
+ * method is called. Since the range of IDs is managed by Zookeeper in
+ * distributed way, this method may block during Zookeeper access.
+ *
+ * @return an IdBlock containing a set of unique IDs
*/
@Override
public IdBlock allocateUniqueIdBlock() {
@@ -563,14 +569,14 @@
SESSION_TIMEOUT, CONNECTION_TIMEOUT, retryPolicy);
curatorFrameworkClient.start();
- curatorFrameworkClient = curatorFrameworkClient.usingNamespace(namespace);
+ curatorFrameworkClient = curatorFrameworkClient.usingNamespace(NAMESPACE);
distributedIdCounter = new DistributedAtomicLong(
curatorFrameworkClient,
ID_COUNTER_PATH,
new RetryOneTime(100));
- rootSwitchCache = new PathChildrenCache(curatorFrameworkClient, switchLatchesPath, true);
+ rootSwitchCache = new PathChildrenCache(curatorFrameworkClient, SWITCH_LATCHES_PATH, true);
rootSwitchCache.getListenable().addListener(switchPathCacheListener);
//Build the service discovery object
@@ -590,11 +596,12 @@
//Don't prime the cache, we want a notification for each child node in the path
rootSwitchCache.start(StartMode.NORMAL);
} catch (Exception e) {
- throw new FloodlightModuleException("Error initialising ZookeeperRegistry: "
- + e.getMessage());
+ throw new FloodlightModuleException(
+ "Error initialising ZookeeperRegistry", e);
}
- eventThreadExecutorService = Executors.newSingleThreadExecutor();
+ ExecutorService eventThreadExecutorService =
+ Executors.newSingleThreadExecutor();
eventThreadExecutorService.execute(
new Runnable() {
@Override