Scrubbing store related TODO/FIXMEs

Change-Id: I4e6bf026845bbd5be127ecacd9956d12f3386c9e

@@ -51,7 +51,7 @@ public interface LinkResourceStore {
      * Returns resources allocated for an Intent.
      *
      * @param intentId the target Intent's ID
-     * @return allocated resources
+     * @return allocated resources or null if no resource is allocated
      */
     LinkResourceAllocations getAllocations(IntentId intentId);
 

@@ -57,7 +57,7 @@ public class LeadershipManager implements LeadershipService {
     // a unexpected error.
     private static final int WAIT_BEFORE_RETRY_MS = 2000;
 
-    // TODO: Appropriate Thread pool sizing.
+    // TODO: Make Thread pool size configurable.
     private final ScheduledExecutorService threadPool =
             Executors.newScheduledThreadPool(25, namedThreads("leadership-manager-%d"));
 

@@ -98,7 +98,6 @@ import static java.util.concurrent.Executors.newSingleThreadScheduledExecutor;
 import static org.onosproject.store.device.impl.GossipDeviceStoreMessageSubjects.DEVICE_ADVERTISE;
 import static org.onosproject.store.device.impl.GossipDeviceStoreMessageSubjects.DEVICE_REMOVE_REQ;
 
-// TODO: give me a better name
 /**
  * Manages inventory of infrastructure devices using gossip protocol to distribute
  * information.
@@ -167,6 +166,10 @@ public class GossipDeviceStore
 
     private ScheduledExecutorService backgroundExecutor;
 
+    // TODO make these anti-entropy parameters configurable
+    private long initialDelaySec = 5;
+    private long periodSec = 5;
+
 
     @Activate
     public void activate() {
@@ -189,9 +192,6 @@ public class GossipDeviceStore
         backgroundExecutor =
                 newSingleThreadScheduledExecutor(minPriority(namedThreads("device-bg-%d")));
 
-        // TODO: Make these configurable
-        long initialDelaySec = 5;
-        long periodSec = 5;
         // start anti-entropy thread
         backgroundExecutor.scheduleAtFixedRate(new SendAdvertisementTask(),
                     initialDelaySec, periodSec, TimeUnit.SECONDS);
@@ -412,7 +412,6 @@ public class GossipDeviceStore
             }
             boolean removed = availableDevices.remove(deviceId);
             if (removed) {
-                // TODO: broadcast ... DOWN only?
                 return new DeviceEvent(DEVICE_AVAILABILITY_CHANGED, device, null);
             }
             return null;
@@ -885,7 +884,7 @@ public class GossipDeviceStore
             if (e.getKey().equals(primary)) {
                 continue;
             }
-            // TODO: should keep track of Description timestamp
+            // Note: should keep track of Description timestamp in the future
             // and only merge conflicting keys when timestamp is newer.
             // Currently assuming there will never be a key conflict between
             // providers
@@ -913,7 +912,6 @@ public class GossipDeviceStore
         ProviderId primary = pickPrimaryPID(descsMap);
         DeviceDescriptions primDescs = descsMap.get(primary);
         // if no primary, assume not enabled
-        // TODO: revisit this default port enabled/disabled behavior
         boolean isEnabled = false;
         DefaultAnnotations annotations = DefaultAnnotations.builder().build();
 
@@ -927,7 +925,7 @@ public class GossipDeviceStore
             if (e.getKey().equals(primary)) {
                 continue;
             }
-            // TODO: should keep track of Description timestamp
+            // Note: should keep track of Description timestamp in the future
             // and only merge conflicting keys when timestamp is newer.
             // Currently assuming there will never be a key conflict between
             // providers
@@ -968,7 +966,6 @@ public class GossipDeviceStore
         return providerDescs.get(pid);
     }
 
-    // TODO: should we be throwing exception?
     private void unicastMessage(NodeId recipient, MessageSubject subject, Object event) throws IOException {
         ClusterMessage message = new ClusterMessage(
                 clusterService.getLocalNode().id(),
@@ -977,7 +974,6 @@ public class GossipDeviceStore
         clusterCommunicator.unicast(message, recipient);
     }
 
-    // TODO: should we be throwing exception?
     private void broadcastMessage(MessageSubject subject, Object event) throws IOException {
         ClusterMessage message = new ClusterMessage(
                 clusterService.getLocalNode().id(),

@@ -17,7 +17,6 @@ package org.onosproject.store.device.impl;
 
 import org.onosproject.store.cluster.messaging.MessageSubject;
 
-// TODO: add prefix to assure uniqueness.
 /**
  * MessageSubjects used by GossipDeviceStore peer-peer communication.
  */

@@ -503,7 +503,7 @@ public class DistributedFlowRuleStore
             }
 
             // TODO: Confirm if this behavior is correct. See SimpleFlowRuleStore
-            // TODO: also update backup.
+            // TODO: also update backup if the behavior is correct.
             flowEntries.put(did, new DefaultFlowEntry(rule));
         } finally {
             flowEntriesLock.writeLock().unlock();
@@ -541,7 +541,7 @@ public class DistributedFlowRuleStore
             Future<byte[]> responseFuture = clusterCommunicator.sendAndReceive(message, replicaInfo.master().get());
             return SERIALIZER.decode(responseFuture.get(FLOW_RULE_STORE_TIMEOUT_MILLIS, TimeUnit.MILLISECONDS));
         } catch (IOException | TimeoutException | ExecutionException | InterruptedException e) {
-            // FIXME: throw a FlowStoreException
+            // TODO: Retry against latest master or throw a FlowStoreException
             throw new RuntimeException(e);
         }
     }
@@ -586,8 +586,6 @@ public class DistributedFlowRuleStore
             for (Entry<FlowId, ImmutableList<StoredFlowEntry>> e
                     : backupFlowTable.entrySet()) {
 
-                // TODO: should we be directly updating internal structure or
-                // should we be triggering event?
                 log.trace("loading {}", e.getValue());
                 for (StoredFlowEntry entry : e.getValue()) {
                     flowEntries.remove(did, entry);
@@ -714,7 +712,7 @@ public class DistributedFlowRuleStore
                     // This node is no longer the master holder,
                     // clean local structure
                     removeFromPrimary(did);
-                    // FIXME: probably should stop pending backup activities in
+                    // TODO: probably should stop pending backup activities in
                     // executors to avoid overwriting with old value
                 }
                 break;
@@ -767,7 +765,6 @@ public class DistributedFlowRuleStore
                     } else {
                         success = backupFlowTable.replace(id, original, newValue);
                     }
-                    // TODO retry?
                     if (!success) {
                         log.error("Updating backup failed.");
                     }
@@ -790,7 +787,6 @@ public class DistributedFlowRuleStore
                     } else {
                         success = backupFlowTable.replace(id, original, newValue);
                     }
-                    // TODO retry?
                     if (!success) {
                         log.error("Updating backup failed.");
                     }

@@ -88,7 +88,6 @@ import com.google.common.collect.Multimap;
 import com.google.common.collect.Multimaps;
 import com.google.common.collect.SetMultimap;
 
-//TODO: multi-provider, annotation not supported.
 /**
  * Manages inventory of end-station hosts in distributed data store
  * that uses optimistic replication and gossip based techniques.

@@ -86,6 +86,4 @@ public class AbsentInvalidatingLoadingCache<K, V> extends
         }
         return v;
     }
-
-    // TODO should we be also checking getAll, etc.
 }

@@ -30,8 +30,6 @@ import java.util.concurrent.TimeUnit;
 
 import static com.google.common.base.Preconditions.checkNotNull;
 
-// TODO: implementation is incomplete
-
 /**
  * Wrapper around IQueue<byte[]> which serializes/deserializes
  * key and value using StoreSerializer.

@@ -28,7 +28,6 @@ import org.onosproject.store.serializers.StoreSerializer;
 import com.hazelcast.core.TransactionalMap;
 import com.hazelcast.query.Predicate;
 
-// TODO: implement Predicate, etc. if we need them.
 /**
  * Wrapper around TransactionalMap<byte[], byte[]> which serializes/deserializes
  * key and value using StoreSerializer.
@@ -100,7 +99,6 @@ public class STxMap<K, V> implements TransactionalMap<K, V> {
 
     @Override
     public V getForUpdate(Object key) {
-        // TODO Auto-generated method stub
         return deserializeVal(m.getForUpdate(serializeKey(key)));
     }
 

@@ -90,7 +90,7 @@ public class DistributedIntentStore
     private static final String STATES_TABLE = "intent-states";
     private CMap<IntentId, IntentState> states;
 
-    // TODO left behind transient state issue: ONOS-103
+    // TODO transient state issue remains for this impl.: ONOS-103
     // Map to store instance local intermediate state transition
     private transient Map<IntentId, IntentState> transientStates = new ConcurrentHashMap<>();
 
@@ -142,7 +142,7 @@ public class DistributedIntentStore
         getIntentTimer = createResponseTimer("getIntent");
         getIntentStateTimer = createResponseTimer("getIntentState");
 
-        // FIXME: We need a way to add serializer for intents which has been plugged-in.
+        // We need a way to add serializer for intents which has been plugged-in.
         // As a short term workaround, relax Kryo config to
         // registrationRequired=false
         serializer = new KryoSerializer() {
@@ -264,7 +264,6 @@ public class DistributedIntentStore
         }
     }
 
-    // FIXME temporary workaround until we fix our state machine
     private void verify(boolean expression, String errorMessageTemplate, Object... errorMessageArgs) {
         if (onlyLogTransitionError) {
             if (!expression) {
@@ -488,7 +487,6 @@ public class DistributedIntentStore
             return failed;
         } else {
             // everything failed
-            // FIXME what to do with events?
             return batch.operations();
         }
     }

@@ -529,7 +529,7 @@ public class GossipLinkStore
                 continue;
             }
 
-            // TODO: should keep track of Description timestamp
+            // Note: In the long run we should keep track of Description timestamp
             // and only merge conflicting keys when timestamp is newer
             // Currently assuming there will never be a key conflict between
             // providers

@@ -299,7 +299,7 @@ public class DistributedMastershipStore
                     } else {
                         // no master candidate
                         roleMap.put(deviceId, rv);
-                        // TODO: Should there be new event type for no MASTER?
+                        // TBD: Should there be new event type for no MASTER?
                         return new MastershipEvent(MASTER_CHANGED, deviceId, rv.roleInfo());
                     }
                 case STANDBY:

@@ -78,7 +78,6 @@ public class DistributedLinkResourceStore implements LinkResourceStore {
 
     private final Logger log = getLogger(getClass());
 
-    // FIXME: what is the Bandwidth unit?
     private static final Bandwidth DEFAULT_BANDWIDTH = Bandwidth.valueOf(1_000);
 
     // table to store current allocations
@@ -143,7 +142,6 @@ public class DistributedLinkResourceStore implements LinkResourceStore {
     }
 
     private Set<? extends ResourceAllocation> getResourceCapacity(ResourceType type, Link link) {
-        // TODO: plugin/provider mechanism to add resource type in the future?
         if (type == ResourceType.BANDWIDTH) {
             return ImmutableSet.of(getBandwidthResourceCapacity(link));
         }
@@ -154,7 +152,6 @@ public class DistributedLinkResourceStore implements LinkResourceStore {
     }
 
     private Set<LambdaResourceAllocation> getLambdaResourceCapacity(Link link) {
-        // FIXME enumerate all the possible link/port lambdas
         Set<LambdaResourceAllocation> allocations = new HashSet<>();
         try {
             final int waves = Integer.parseInt(link.annotations().value(wavesAnnotation));
@@ -323,7 +320,6 @@ public class DistributedLinkResourceStore implements LinkResourceStore {
             if (log.isDebugEnabled()) {
                 logFailureDetail(batch, result);
             }
-            // FIXME throw appropriate exception, with what failed.
             checkState(result.isSuccessful(), "Allocation failed");
         }
     }
@@ -389,7 +385,6 @@ public class DistributedLinkResourceStore implements LinkResourceStore {
                 double bwLeft = bw.bandwidth().toDouble();
                 bwLeft -= ((BandwidthResourceAllocation) req).bandwidth().toDouble();
                 if (bwLeft < 0) {
-                    // FIXME throw appropriate Exception
                     checkState(bwLeft >= 0,
                                "There's no Bandwidth left on %s. %s",
                                link, bwLeft);
@@ -399,7 +394,6 @@ public class DistributedLinkResourceStore implements LinkResourceStore {
                 // check if allocation should be accepted
                 if (!avail.contains(req)) {
                     // requested lambda was not available
-                    // FIXME throw appropriate exception
                     checkState(avail.contains(req),
                                "Allocating %s on %s failed",
                                req, link);
@@ -433,7 +427,6 @@ public class DistributedLinkResourceStore implements LinkResourceStore {
         final String dbIntentId = toIntentDbKey(intendId);
         final Collection<Link> links = allocations.links();
 
-        // TODO: does release must happen in a batch?
         boolean success;
         do {
             Builder tx = BatchWriteRequest.newBuilder();
@@ -476,7 +469,6 @@ public class DistributedLinkResourceStore implements LinkResourceStore {
         checkNotNull(intentId);
         VersionedValue vv = databaseService.get(INTENT_ALLOCATIONS, toIntentDbKey(intentId));
         if (vv == null) {
-            // FIXME: should we return null or LinkResourceAllocations with nothing allocated?
             return null;
         }
         LinkResourceAllocations allocations = decodeIntentAllocations(vv.value());
@@ -486,7 +478,7 @@ public class DistributedLinkResourceStore implements LinkResourceStore {
     private String toLinkDbKey(LinkKey linkid) {
         // introduce cache if necessary
         return linkid.toString();
-        // TODO: Above is irreversible, if we need reverse conversion
+        // Note: Above is irreversible, if we need reverse conversion
         // we may need something like below, due to String only limitation
 //        byte[] bytes = serializer.encode(linkid);
 //        StringBuilder builder = new StringBuilder(bytes.length * 4);

@@ -76,7 +76,6 @@ public class HazelcastLinkResourceStore
 
     private final Logger log = getLogger(getClass());
 
-    // FIXME: what is the Bandwidth unit?
     private static final Bandwidth DEFAULT_BANDWIDTH = Bandwidth.valueOf(1_000);
 
     private static final Bandwidth EMPTY_BW = Bandwidth.valueOf(0);
@@ -134,7 +133,6 @@ public class HazelcastLinkResourceStore
     }
 
     private Set<? extends ResourceAllocation> getResourceCapacity(ResourceType type, Link link) {
-        // TODO: plugin/provider mechanism to add resource type in the future?
         if (type == ResourceType.BANDWIDTH) {
             return ImmutableSet.of(getBandwidthResourceCapacity(link));
         }
@@ -145,7 +143,6 @@ public class HazelcastLinkResourceStore
     }
 
     private Set<LambdaResourceAllocation> getLambdaResourceCapacity(Link link) {
-        // FIXME enumerate all the possible link/port lambdas
         Set<LambdaResourceAllocation> allocations = new HashSet<>();
         try {
             final int waves = Integer.parseInt(link.annotations().value(wavesAnnotation));
@@ -335,7 +332,6 @@ public class HazelcastLinkResourceStore
                 double bwLeft = bw.bandwidth().toDouble();
                 bwLeft -= ((BandwidthResourceAllocation) req).bandwidth().toDouble();
                 if (bwLeft < 0) {
-                    // FIXME throw appropriate Exception
                     checkState(bwLeft >= 0,
                                "There's no Bandwidth left on %s. %s",
                                link, bwLeft);
@@ -345,7 +341,6 @@ public class HazelcastLinkResourceStore
                 // check if allocation should be accepted
                 if (!avail.contains(req)) {
                     // requested lambda was not available
-                    // FIXME throw appropriate exception
                     checkState(avail.contains(req),
                                "Allocating %s on %s failed",
                                req, link);
@@ -381,7 +376,8 @@ public class HazelcastLinkResourceStore
 
         boolean success = false;
         do {
-            // TODO: smaller tx unit to lower the chance of collisions?
+            // Note: might want to break it down into smaller tx unit
+            // to lower the chance of collisions.
             TransactionContext tx = theInstance.newTransactionContext();
             tx.beginTransaction();
             try {

@@ -100,7 +100,7 @@ public class ClusterMessagingProtocol
                      // for snapshot
                     .register(State.class)
                     .register(TableMetadata.class)
-                    // TODO: Move this out ?
+                    // TODO: Move this out to API?
                     .register(TableModificationEvent.class)
                     .register(TableModificationEvent.Type.class)
                     .build();

@@ -96,13 +96,11 @@ public class DistributedLockManager implements LockService {
 
     @Override
     public void addListener(LockEventListener listener) {
-        // FIXME:
         throw new UnsupportedOperationException();
     }
 
     @Override
     public void removeListener(LockEventListener listener) {
-        // FIXME:
         throw new UnsupportedOperationException();
     }