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/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