Refactoring to eliminate duplicate DefaultTopology and DefaultTopologyGraph; eli…

…minating a few fixmes.

Change-Id: Ie0e38dbf812bafdb7c94bba5278f0dd9af5be929

@@ -474,11 +474,10 @@ public class ReactiveForwarding {
 
 
             // Otherwise, pick a path that does not lead back to where we
             // Otherwise, pick a path that does not lead back to where we
             // came from; if no such path, flood and bail.
             // came from; if no such path, flood and bail.
-            Path path = pickForwardPath(paths, pkt.receivedFrom().port());
+            Path path = pickForwardPathIfPossible(paths, pkt.receivedFrom().port());
             if (path == null) {
             if (path == null) {
-                log.warn("Doh... don't know where to go... {} -> {} received on {}",
-                         ethPkt.getSourceMAC(), ethPkt.getDestinationMAC(),
-                         pkt.receivedFrom());
+                log.warn("Don't know where to go from here {} for {} -> {}",
+                         pkt.receivedFrom(), ethPkt.getSourceMAC(), ethPkt.getDestinationMAC());
                 flood(context);
                 flood(context);
                 return;
                 return;
             }
             }
@@ -501,14 +500,16 @@ public class ReactiveForwarding {
     }
     }
 
 
     // Selects a path from the given set that does not lead back to the
     // Selects a path from the given set that does not lead back to the
-    // specified port.
-    private Path pickForwardPath(Set<Path> paths, PortNumber notToPort) {
+    // specified port if possible.
+    private Path pickForwardPathIfPossible(Set<Path> paths, PortNumber notToPort) {
+        Path lastPath = null;
         for (Path path : paths) {
         for (Path path : paths) {
+            lastPath = path;
             if (!path.src().port().equals(notToPort)) {
             if (!path.src().port().equals(notToPort)) {
                 return path;
                 return path;
             }
             }
         }
         }
-        return null;
+        return lastPath;
     }
     }
 
 
     // Floods the specified packet if permissible.
     // Floods the specified packet if permissible.
@@ -734,7 +735,7 @@ public class ReactiveForwarding {
                 Set<Path> pathsFromCurDevice =
                 Set<Path> pathsFromCurDevice =
                         topologyService.getPaths(topologyService.currentTopology(),
                         topologyService.getPaths(topologyService.currentTopology(),
                                                  curDevice, dstId);
                                                  curDevice, dstId);
-                if (pickForwardPath(pathsFromCurDevice, curLink.src().port()) != null) {
+                if (pickForwardPathIfPossible(pathsFromCurDevice, curLink.src().port()) != null) {
                     break;
                     break;
                 } else {
                 } else {
                     if (i + 1 == pathLinks.size()) {
                     if (i + 1 == pathLinks.size()) {
@@ -792,8 +793,8 @@ public class ReactiveForwarding {
         return builder.build();
         return builder.build();
     }
     }
 
 
-    // Returns set of flowEntries which were created by this application and which egress from the
-    // specified connection port
+    // Returns set of flow entries which were created by this application and
+    // which egress from the specified connection port
     private Set<FlowEntry> getFlowRulesFrom(ConnectPoint egress) {
     private Set<FlowEntry> getFlowRulesFrom(ConnectPoint egress) {
         ImmutableSet.Builder<FlowEntry> builder = ImmutableSet.builder();
         ImmutableSet.Builder<FlowEntry> builder = ImmutableSet.builder();
         flowRuleService.getFlowEntries(egress.deviceId()).forEach(r -> {
         flowRuleService.getFlowEntries(egress.deviceId()).forEach(r -> {

@@ -27,6 +27,7 @@ import org.slf4j.Logger;
 
 
 import java.util.Map;
 import java.util.Map;
 
 
+import static com.google.common.base.Preconditions.checkArgument;
 import static org.slf4j.LoggerFactory.getLogger;
 import static org.slf4j.LoggerFactory.getLogger;
 
 
 /**
 /**
@@ -133,9 +134,7 @@ public class DefaultGraphDescription extends AbstractDescription
     private TopologyVertex vertexOf(ConnectPoint connectPoint) {
     private TopologyVertex vertexOf(ConnectPoint connectPoint) {
         DeviceId id = connectPoint.deviceId();
         DeviceId id = connectPoint.deviceId();
         TopologyVertex vertex = vertexesById.get(id);
         TopologyVertex vertex = vertexesById.get(id);
-        if (vertex == null) {
-            throw new IllegalArgumentException("Vertex missing for " + id);
-        }
+        checkArgument(vertex != null, "Vertex missing for %s", id);
         return vertex;
         return vertex;
     }
     }
 
 

@@ -50,7 +50,9 @@ import java.util.Map;
 import java.util.Set;
 import java.util.Set;
 
 
 import static com.google.common.base.MoreObjects.toStringHelper;
 import static com.google.common.base.MoreObjects.toStringHelper;
+import static com.google.common.base.Preconditions.checkArgument;
 import static org.onlab.graph.GraphPathSearch.ALL_PATHS;
 import static org.onlab.graph.GraphPathSearch.ALL_PATHS;
+import static org.onlab.util.Tools.isNullOrEmpty;
 import static org.onosproject.core.CoreService.CORE_PROVIDER_ID;
 import static org.onosproject.core.CoreService.CORE_PROVIDER_ID;
 import static org.onosproject.net.Link.State.ACTIVE;
 import static org.onosproject.net.Link.State.ACTIVE;
 import static org.onosproject.net.Link.State.INACTIVE;
 import static org.onosproject.net.Link.State.INACTIVE;
@@ -228,15 +230,12 @@ public class DefaultTopology extends AbstractModel implements Topology {
 
 
         // Find the cluster to which the device belongs.
         // Find the cluster to which the device belongs.
         TopologyCluster cluster = clustersByDevice().get(connectPoint.deviceId());
         TopologyCluster cluster = clustersByDevice().get(connectPoint.deviceId());
-        if (cluster == null) {
-            throw new IllegalArgumentException("No cluster found for device "
-                                                       + connectPoint.deviceId());
-        }
+        checkArgument(cluster != null, "No cluster found for device %s", connectPoint.deviceId());
 
 
         // If the broadcast set is null or empty, or if the point explicitly
         // If the broadcast set is null or empty, or if the point explicitly
-        // belongs to it, return true;
+        // belongs to it, return true.
         Set<ConnectPoint> points = broadcastSets.get().get(cluster.id());
         Set<ConnectPoint> points = broadcastSets.get().get(cluster.id());
-        return (points == null) || points.isEmpty() || points.contains(connectPoint);
+        return isNullOrEmpty(points) || points.contains(connectPoint);
     }
     }
 
 
     /**
     /**
@@ -250,6 +249,16 @@ public class DefaultTopology extends AbstractModel implements Topology {
     }
     }
 
 
     /**
     /**
+     * Returns the set of the cluster broadcast points.
+     *
+     * @param clusterId cluster identifier
+     * @return set of cluster broadcast points
+     */
+    public Set<ConnectPoint> broadcastPoints(ClusterId clusterId) {
+        return broadcastSets.get().get(clusterId);
+    }
+
+    /**
      * Returns the set of pre-computed shortest paths between source and
      * Returns the set of pre-computed shortest paths between source and
      * destination devices.
      * destination devices.
      *
      *

@@ -279,8 +279,7 @@ public class DefaultTopologyProvider extends AbstractProvider
             try {
             try {
                 buildTopology(reasons);
                 buildTopology(reasons);
             } catch (Exception e) {
             } catch (Exception e) {
-                log.warn("Unable to compute topology due to: {}", e.getMessage());
-                log.debug("Unable to compute topology", e);
+                log.warn("Unable to compute topology", e);
             }
             }
         }
         }
     }
     }

@@ -65,7 +65,7 @@ public class DistributedTopologyStore
 
 
     private volatile DefaultTopology current =
     private volatile DefaultTopology current =
             new DefaultTopology(ProviderId.NONE,
             new DefaultTopology(ProviderId.NONE,
-                                new DefaultGraphDescription(0L, 0L,
+                                new DefaultGraphDescription(0L, System.currentTimeMillis(),
                                                             Collections.<Device>emptyList(),
                                                             Collections.<Device>emptyList(),
                                                             Collections.<Link>emptyList()));
                                                             Collections.<Link>emptyList()));
 
 
@@ -147,8 +147,7 @@ public class DistributedTopologyStore
         }
         }
 
 
         // Have the default topology construct self from the description data.
         // Have the default topology construct self from the description data.
-        DefaultTopology newTopology =
-                new DefaultTopology(providerId, graphDescription);
+        DefaultTopology newTopology = new DefaultTopology(providerId, graphDescription);
 
 
         // Promote the new topology to current and return a ready-to-send event.
         // Promote the new topology to current and return a ready-to-send event.
         synchronized (this) {
         synchronized (this) {