Checkstyle and pmd fixes for new MatchAction service
Change-Id: I13efd4c7a29e19b782c126132d151ac377db3a4c
diff --git a/src/main/java/net/onrc/onos/core/flowprogrammer/FlowPusher.java b/src/main/java/net/onrc/onos/core/flowprogrammer/FlowPusher.java
index 824ed4f..27f1a3c 100644
--- a/src/main/java/net/onrc/onos/core/flowprogrammer/FlowPusher.java
+++ b/src/main/java/net/onrc/onos/core/flowprogrammer/FlowPusher.java
@@ -739,7 +739,7 @@
.setActions(actionList)
.setIdleTimeout(0) // hardcoded to zero for now
.setHardTimeout(0) // hardcoded to zero for now
- .setCookie(U64.of(matchAction.getId().value()))
+ .setCookie(U64.of(matchAction.getId().getValue()))
.setBufferId(OFBufferId.NO_BUFFER)
.setPriority(PRIORITY_DEFAULT)
.setOutPort(outp);
diff --git a/src/main/java/net/onrc/onos/core/matchaction/MatchAction.java b/src/main/java/net/onrc/onos/core/matchaction/MatchAction.java
index fdf1fc7..d7d5d73 100644
--- a/src/main/java/net/onrc/onos/core/matchaction/MatchAction.java
+++ b/src/main/java/net/onrc/onos/core/matchaction/MatchAction.java
@@ -42,21 +42,6 @@
}
/**
- * Constructor. TEMPORARY
- *
- * @param id ID for this MatchAction object
- * @param port switch port to apply changes to
- * @param match the Match object as match condition on the port
- * @param actions the list of Action objects as actions on the switch
- */
- public MatchAction(String id, SwitchPort port, Match match, List<Action> actions) {
- this.id = null;
- this.port = port;
- this.match = match;
- this.actions = actions;
- }
-
- /**
* Gets ID for this object.
*
* @return the ID for this object
diff --git a/src/main/java/net/onrc/onos/core/matchaction/MatchActionComponent.java b/src/main/java/net/onrc/onos/core/matchaction/MatchActionComponent.java
index da24d4d..0307d4c 100644
--- a/src/main/java/net/onrc/onos/core/matchaction/MatchActionComponent.java
+++ b/src/main/java/net/onrc/onos/core/matchaction/MatchActionComponent.java
@@ -40,29 +40,33 @@
public class MatchActionComponent implements MatchActionService, IFloodlightService {
private static final Logger log = LoggerFactory.getLogger(MatchActionService.class);
- IFlowPusherService pusher;
- IFloodlightProviderService provider;
+ private final IFlowPusherService pusher;
+ private final IFloodlightProviderService provider;
- private ConcurrentMap<MatchActionId, MatchAction> matchActionMap = new ConcurrentHashMap<>();
- private ConcurrentMap<MatchActionOperationsId, MatchActionOperations> matchSetMap =
+ private final ConcurrentMap<MatchActionId, MatchAction> matchActionMap = new ConcurrentHashMap<>();
+ private final ConcurrentMap<MatchActionOperationsId, MatchActionOperations> matchSetMap =
new ConcurrentHashMap<>();
// TODO - want something better here for the resolved Queue
- private BlockingQueue<MatchActionOperationsId> resolvedQueue = new ArrayBlockingQueue<>(100);
- private BlockingQueue<MatchActionOperations> installationWorkQueue = new ArrayBlockingQueue<>(100);
+ private final BlockingQueue<MatchActionOperationsId> resolvedQueue = new ArrayBlockingQueue<>(100);
+ private final BlockingQueue<MatchActionOperations> installationWorkQueue = new ArrayBlockingQueue<>(100);
private IEventChannel<String, MatchActionOperations> installSetChannel;
private IEventChannel<String, SwitchResultList> installSetReplyChannel;
- // TODO Single instance for now, should be a work queue of some sort eventually
- private Thread coordinator;
- private Thread installer;
- private Installer installerListener;
private final IDatagridService datagrid;
- private IControllerRegistryService registryService;
+ private final IControllerRegistryService registryService;
private MatchActionIdGeneratorWithIdBlockAllocator matchActionIdGenerator;
private MatchActionOperationsIdGeneratorWithIdBlockAllocator matchActionOperationsIdGenerator;
+ /**
+ * Constructs a MatchActionComponent given the services it depends on.
+ *
+ * @param newDatagrid datagrid dependency
+ * @param newPusher flow pusher dependency
+ * @param newProvider provider used for switch queries
+ * @param newRegistryService registry for ID block allocation
+ */
public MatchActionComponent(final IDatagridService newDatagrid,
final IFlowPusherService newPusher,
final IFloodlightProviderService newProvider,
@@ -73,6 +77,10 @@
registryService = newRegistryService;
}
+ /**
+ * Starts the component. Created channels used for communication and
+ * creates producer and consumer threads.
+ */
public void start() {
IdBlockAllocator idBlockAllocator = registryService;
matchActionIdGenerator =
@@ -88,16 +96,23 @@
String.class,
SwitchResultList.class);
- coordinator = new Coordinator();
+ final Thread coordinator = new Coordinator();
coordinator.start();
- installer = new InstallerWorker();
+ // TODO Single instance for now, should be a work queue of some sort eventually
+ final Thread installer = new InstallerWorker();
installer.start();
- installerListener = new Installer();
+ final Installer installerListener = new Installer();
installerListener.start();
}
+ /**
+ * Installs a set of MatchActionOperations.
+ *
+ * @param matchSet the set of MatchActions to install
+ * @return identifier of the installed operations
+ */
public MatchActionOperationsId installMatchActionOperations(MatchActionOperations matchSet) {
if (checkResolved(matchSet)) {
matchSet.setState(MatchActionOperationsState.RESOLVED);
@@ -115,12 +130,24 @@
return matchSet.getOperationsId();
}
+ /**
+ * Returns the state of a set of operations.
+ *
+ * @param matchSetId identifier of the MatchActionOperations being queried.
+ * @return state of the given operations
+ */
public MatchActionOperationsState getMatchActionOperationsState(MatchActionOperationsId matchSetId) {
MatchActionOperations set = matchSetMap.get(matchSetId);
return (set == null) ? null : set.getState();
}
- protected boolean checkResolved(MatchActionOperations matchSet) {
+ /**
+ * Checks if a given set of operations has all of its dependencies resolved.
+ *
+ * @param matchSet Operations set to check
+ * @return true if all dependencies are resolved, false otherwise
+ */
+ private boolean checkResolved(MatchActionOperations matchSet) {
boolean resolved = true;
for (MatchActionOperationsId setId : matchSet.getDependencies()) {
MatchActionOperations set = matchSetMap.get(setId);
@@ -132,17 +159,28 @@
return resolved;
}
- class Coordinator extends Thread
+ /**
+ * Producer class for MatchActionOperations. An instance of this runs on
+ * each ONOS node. Requests come in via the resolved queue, and are
+ * distributed to workers running on each ONOS instance via a channel.
+ */
+ private final class Coordinator extends Thread
implements IEventChannelListener<String, SwitchResultList> {
- private Map<MatchActionOperationsId, Map<Dpid, SwitchResult>> pendingMatchActionOperations = new HashMap<>();
+ private final Map<MatchActionOperationsId,
+ Map<Dpid, SwitchResult>>
+ pendingMatchActionOperations = new HashMap<>();
- protected Coordinator() {
+ /**
+ * Default constructor.
+ */
+ Coordinator() {
installSetReplyChannel.addListener(this);
}
@Override
public void run() {
+ //noinspection InfiniteLoopStatement - for IntelliJ
while (true) {
// 1. Remove MatchActionOperations(s) from the Global Resolved Queue
try {
@@ -154,6 +192,11 @@
}
}
+ /**
+ * Processes an inbound MatchActionOperations object.
+ *
+ * @param setId Identifier of the MatchActionOperations object
+ */
private void processSet(MatchActionOperationsId setId) {
MatchActionOperations matchSet = matchSetMap.get(setId);
matchSet.setState(MatchActionOperationsState.PENDING);
@@ -203,6 +246,11 @@
updateSwitchResults(value);
}
+ /**
+ * Processes the response from a consumer.
+ *
+ * @param results List of switches modified by the consumer
+ */
private void updateSwitchResults(SwitchResultList results) {
if (results == null || results.size() == 0) {
return;
@@ -262,11 +310,28 @@
}
}
- class InstallerWorker extends Thread {
+ /**
+ * Worker thread that pushes MatchActionOperations to the switches via
+ * the FlowPusher.
+ */
+ private class InstallerWorker extends Thread {
+
+ /**
+ * Default constructor.
+ */
+ InstallerWorker() {
+ // nothing to initialize
+ }
// Note: we should consider using an alternative representation for
// apply sets
- protected void install(MatchActionOperations matchSet) {
+
+ /**
+ * Installs a set of MatchActionOperations using the Flow Pusher.
+ *
+ * @param matchSet set of MatchActions to install
+ */
+ private void install(MatchActionOperations matchSet) {
Set<Long> masterDpids = provider.getAllMasterSwitchDpids();
Set<MatchActionOperationEntry> installSet = new HashSet<>();
@@ -320,6 +385,7 @@
@Override
public void run() {
+ //noinspection InfiniteLoopStatement - for IntelliJ
while (true) {
// 1. Remove MatchActionOperations(s) from the Global Resolved Queue
try {
@@ -332,10 +398,19 @@
}
}
+ /**
+ * Consumer class for MatchActionOperations. Listens on the MatchAction
+ * channel and places inbound requests on a queue to be handled by the
+ * InstallerWorker threads.
+ */
class Installer
implements IEventChannelListener<String, MatchActionOperations> {
- protected void start() {
+ /**
+ * Starts the Installer consumer. Adds a listener on the MatchActionOperations
+ * channel.
+ */
+ private void start() {
installSetChannel.addListener(this);
}
diff --git a/src/main/java/net/onrc/onos/core/matchaction/MatchActionId.java b/src/main/java/net/onrc/onos/core/matchaction/MatchActionId.java
index bea973e..59579a5 100644
--- a/src/main/java/net/onrc/onos/core/matchaction/MatchActionId.java
+++ b/src/main/java/net/onrc/onos/core/matchaction/MatchActionId.java
@@ -31,7 +31,7 @@
*
* @return MatchAction ID
*/
- public long value() {
+ public long getValue() {
return value;
}
diff --git a/src/main/java/net/onrc/onos/core/matchaction/MatchActionModule.java b/src/main/java/net/onrc/onos/core/matchaction/MatchActionModule.java
index 4970cf1..9bf1be4 100644
--- a/src/main/java/net/onrc/onos/core/matchaction/MatchActionModule.java
+++ b/src/main/java/net/onrc/onos/core/matchaction/MatchActionModule.java
@@ -4,7 +4,6 @@
import java.util.Collection;
import java.util.EventListener;
import java.util.HashMap;
-import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
@@ -31,7 +30,6 @@
public class MatchActionModule implements MatchActionFloodlightService, IFloodlightModule {
- private final HashSet<MatchAction> currentOperations = new HashSet<>();
private static final Logger log = LoggerFactory
.getLogger(MatchActionModule.class);
private MatchActionComponent matchActionComponent;
@@ -99,7 +97,7 @@
@Override
public void setConflictDetectionPolicy(ConflictDetectionPolicy policy) {
-
+ throw new UnsupportedOperationException("conflict detection not implemented yet");
}
@Override
@@ -119,11 +117,11 @@
@Override
public void addEventListener(EventListener listener) {
-
+ throw new UnsupportedOperationException("events not implemented yet");
}
@Override
public void removeEventListener(EventListener listener) {
-
+ throw new UnsupportedOperationException("events not implemented yet");
}
}
diff --git a/src/main/java/net/onrc/onos/core/matchaction/MatchActionOperations.java b/src/main/java/net/onrc/onos/core/matchaction/MatchActionOperations.java
index 73cff28..7d9063f 100644
--- a/src/main/java/net/onrc/onos/core/matchaction/MatchActionOperations.java
+++ b/src/main/java/net/onrc/onos/core/matchaction/MatchActionOperations.java
@@ -22,7 +22,10 @@
* The MatchAction operators.
*/
public enum Operator {
+ /** Add a new match action. */
ADD,
+
+ /** Remove an existing match action. */
REMOVE,
}
@@ -83,6 +86,12 @@
return dependencies;
}
+ /**
+ * Adds a dependency to this set of Operations.
+ *
+ * @param dependentOperationId Identifier of the Operations that must
+ * complete before this one can be installed
+ */
public void addDependency(MatchActionOperationsId dependentOperationId) {
dependencies.add(dependentOperationId);
}
diff --git a/src/main/java/net/onrc/onos/core/matchaction/MatchActionOperationsId.java b/src/main/java/net/onrc/onos/core/matchaction/MatchActionOperationsId.java
index 93f2ec7..a05dd90 100644
--- a/src/main/java/net/onrc/onos/core/matchaction/MatchActionOperationsId.java
+++ b/src/main/java/net/onrc/onos/core/matchaction/MatchActionOperationsId.java
@@ -13,6 +13,8 @@
/**
* Constructs an Operations identifier and from a unique identifier.
+ *
+ * @param newId unique identifier to use for the new Id object
*/
public MatchActionOperationsId(final long newId) {
id = newId;
diff --git a/src/main/java/net/onrc/onos/core/matchaction/MatchActionOperationsState.java b/src/main/java/net/onrc/onos/core/matchaction/MatchActionOperationsState.java
index 4ccf668..f0470af 100644
--- a/src/main/java/net/onrc/onos/core/matchaction/MatchActionOperationsState.java
+++ b/src/main/java/net/onrc/onos/core/matchaction/MatchActionOperationsState.java
@@ -1,11 +1,22 @@
package net.onrc.onos.core.matchaction;
-
+/**
+ * States for MatchActionOperations objects.
+ */
public enum MatchActionOperationsState {
+ /** Being initialized. */
INIT,
+
+ /** All operations that we depend on are finished. */
RESOLVED,
+
+ /** Operation is pending waiting for dependencies. */
PENDING,
+
+ /** Operations successfully installed. */
INSTALLED,
+
+ /** Operations installation failed. */
FAILED
}
diff --git a/src/main/java/net/onrc/onos/core/matchaction/MatchActionStatus.java b/src/main/java/net/onrc/onos/core/matchaction/MatchActionStatus.java
index 0a7c145..a1f46f4 100644
--- a/src/main/java/net/onrc/onos/core/matchaction/MatchActionStatus.java
+++ b/src/main/java/net/onrc/onos/core/matchaction/MatchActionStatus.java
@@ -1,5 +1,8 @@
package net.onrc.onos.core.matchaction;
+/**
+ * Placeholder.
+ */
public interface MatchActionStatus {
}
diff --git a/src/main/java/net/onrc/onos/core/matchaction/SwitchResult.java b/src/main/java/net/onrc/onos/core/matchaction/SwitchResult.java
index b5db2e8..7f9e18a 100644
--- a/src/main/java/net/onrc/onos/core/matchaction/SwitchResult.java
+++ b/src/main/java/net/onrc/onos/core/matchaction/SwitchResult.java
@@ -2,17 +2,34 @@
import net.onrc.onos.core.util.Dpid;
+/**
+ * The result of applying a MatchAction operation to a switch.
+ */
public class SwitchResult {
private Dpid sw;
private Status status;
private MatchActionOperationsId matchSetId;
+ /**
+ * Status of the switch operation.
+ */
public enum Status {
+ /** Installation of the MatchAction was successful. */
SUCCESS,
+
+ /** Installation of the MatchAction failed. */
FAILURE,
+
+ /** No status has been assigned. */
UNKNOWN
}
+ /**
+ * Creates a new SwitchResult object.
+ *
+ * @param match identifier for MatchActionsOperations that was requested
+ * @param sw Dpid of the switch that the operations were applied to
+ */
protected SwitchResult(MatchActionOperationsId match, Dpid sw) {
this.sw = sw;
this.status = Status.UNKNOWN;
@@ -23,21 +40,41 @@
* no-arg constructor for Kryo.
*/
protected SwitchResult() {
-
+ // Needed for Kryo
}
+ /**
+ * Sets the status of the SwitchResult.
+ *
+ * @param newStatus new status
+ */
protected void setStatus(Status newStatus) {
this.status = newStatus;
}
+ /**
+ * Gets the status of the SwitchResult.
+ *
+ * @return status
+ */
protected Status getStatus() {
return this.status;
}
+ /**
+ * Gets the identifier for the set of operations that was requested.
+ *
+ * @return MatchActionOperationsId of the requested set of operations
+ */
protected MatchActionOperationsId getMatchActionOperationsId() {
return this.matchSetId;
}
+ /**
+ * Gets the identifier for the switch that was requested.
+ *
+ * @return Dpid of the requested switch
+ */
protected Dpid getSwitch() {
return this.sw;
}
diff --git a/src/main/java/net/onrc/onos/core/matchaction/SwitchResultList.java b/src/main/java/net/onrc/onos/core/matchaction/SwitchResultList.java
index 2523556..ff68df2 100644
--- a/src/main/java/net/onrc/onos/core/matchaction/SwitchResultList.java
+++ b/src/main/java/net/onrc/onos/core/matchaction/SwitchResultList.java
@@ -9,13 +9,5 @@
static final long serialVersionUID = -4966789015808022563L;
- /**
- * Add a switch result to the list.
- *
- * @param result switch result to add to the list
- * @return true
- */
- public boolean add(SwitchResult result) {
- return super.add(result);
- }
+
}
diff --git a/src/main/java/net/onrc/onos/core/matchaction/match/OpticalMatch.java b/src/main/java/net/onrc/onos/core/matchaction/match/OpticalMatch.java
index 3cdec64..65c080b 100644
--- a/src/main/java/net/onrc/onos/core/matchaction/match/OpticalMatch.java
+++ b/src/main/java/net/onrc/onos/core/matchaction/match/OpticalMatch.java
@@ -9,7 +9,7 @@
public class OpticalMatch implements Match {
// Match fields
- protected Integer srcLambda;
+ private Integer srcLambda;
/**
* Constructor.
diff --git a/src/main/java/net/onrc/onos/core/matchaction/match/PacketMatchBuilder.java b/src/main/java/net/onrc/onos/core/matchaction/match/PacketMatchBuilder.java
index 1ddc720..7f8be18 100644
--- a/src/main/java/net/onrc/onos/core/matchaction/match/PacketMatchBuilder.java
+++ b/src/main/java/net/onrc/onos/core/matchaction/match/PacketMatchBuilder.java
@@ -8,14 +8,14 @@
* A builder to instantiate PacketMatch class.
*/
public class PacketMatchBuilder {
- private MACAddress srcMac = null;
- private MACAddress dstMac = null;
- private Short etherType = null;
- private IPv4Net srcIp = null;
- private IPv4Net dstIp = null;
- private Byte ipProto = null;
- private Short srcTcpPort = null;
- private Short dstTcpPort = null;
+ private MACAddress srcMac;
+ private MACAddress dstMac;
+ private Short etherType;
+ private IPv4Net srcIp;
+ private IPv4Net dstIp;
+ private Byte ipProto;
+ private Short srcTcpPort;
+ private Short dstTcpPort;
/**
* Sets source MAC address.
diff --git a/src/main/java/net/onrc/onos/core/matchaction/web/MatchActionResource.java b/src/main/java/net/onrc/onos/core/matchaction/web/MatchActionResource.java
index 20e68d3..ca3f95e 100644
--- a/src/main/java/net/onrc/onos/core/matchaction/web/MatchActionResource.java
+++ b/src/main/java/net/onrc/onos/core/matchaction/web/MatchActionResource.java
@@ -22,7 +22,7 @@
*/
public class MatchActionResource extends ServerResource {
- CustomSerializerHelper matchActionSerializers;
+ private final CustomSerializerHelper matchActionSerializers;
/**
* Constructs a MatchActionResource.
diff --git a/src/test/java/net/onrc/onos/core/matchaction/MatchActionIdGeneratorTest.java b/src/test/java/net/onrc/onos/core/matchaction/MatchActionIdGeneratorTest.java
index cb4ed35..321f65d 100644
--- a/src/test/java/net/onrc/onos/core/matchaction/MatchActionIdGeneratorTest.java
+++ b/src/test/java/net/onrc/onos/core/matchaction/MatchActionIdGeneratorTest.java
@@ -17,6 +17,9 @@
public class MatchActionIdGeneratorTest {
private IdBlockAllocator allocator;
+ /**
+ * Creates a mocked ID Block allocator.
+ */
@Before
public void setUp() {
allocator = createMock(IdBlockAllocator.class);
diff --git a/src/test/java/net/onrc/onos/core/matchaction/MatchActionModuleTest.java b/src/test/java/net/onrc/onos/core/matchaction/MatchActionModuleTest.java
index 42e5475..0949cca 100644
--- a/src/test/java/net/onrc/onos/core/matchaction/MatchActionModuleTest.java
+++ b/src/test/java/net/onrc/onos/core/matchaction/MatchActionModuleTest.java
@@ -29,6 +29,9 @@
private IDatagridService datagridService;
private FloodlightModuleContext modContext;
+ /**
+ * Sets up the mocks used by the test.
+ */
@Before
@SuppressWarnings("unchecked")
public void setUpMocks() {
diff --git a/src/test/java/net/onrc/onos/core/matchaction/MatchActionOperationsIdGeneratorTest.java b/src/test/java/net/onrc/onos/core/matchaction/MatchActionOperationsIdGeneratorTest.java
index e95f1df..6dc04c8 100644
--- a/src/test/java/net/onrc/onos/core/matchaction/MatchActionOperationsIdGeneratorTest.java
+++ b/src/test/java/net/onrc/onos/core/matchaction/MatchActionOperationsIdGeneratorTest.java
@@ -18,6 +18,9 @@
private IdBlockAllocator allocator;
+ /**
+ * Creates a mocked IdBlockAllocator.
+ */
@Before
public void setUp() {
allocator = createMock(IdBlockAllocator.class);
diff --git a/src/test/java/net/onrc/onos/core/matchaction/MatchActionTest.java b/src/test/java/net/onrc/onos/core/matchaction/MatchActionTest.java
index 2684f19..0c8bdf6 100644
--- a/src/test/java/net/onrc/onos/core/matchaction/MatchActionTest.java
+++ b/src/test/java/net/onrc/onos/core/matchaction/MatchActionTest.java
@@ -13,8 +13,14 @@
import org.junit.Test;
+/**
+ * Unit tests for MatchAction objects.
+ */
public class MatchActionTest {
+ /**
+ * Tests that a MatchAction object can be constructed properly.
+ */
@Test
public void testConstructor() {
SwitchPort port = new SwitchPort(123L, (short) 55);