Immutability for getMeters APIs

Change-Id: Iaf908766aa360e84e82306e398fff56c9593d8f1
diff --git a/core/api/src/main/java/org/onosproject/net/meter/MeterStore.java b/core/api/src/main/java/org/onosproject/net/meter/MeterStore.java
index 3f0efa0..675592a 100644
--- a/core/api/src/main/java/org/onosproject/net/meter/MeterStore.java
+++ b/core/api/src/main/java/org/onosproject/net/meter/MeterStore.java
@@ -70,8 +70,9 @@
      * Updates a given meter's state with the provided state.
      *
      * @param meter a meter
+     * @return the updated meter
      */
-    void updateMeterState(Meter meter);
+    Meter updateMeterState(Meter meter);
 
     /**
      * Obtains a meter matching the given meter key.
@@ -84,7 +85,7 @@
     /**
      * Returns all meters stored in the store.
      *
-     * @return a collection of meters
+     * @return an immutable copy of all meters
      */
     Collection<Meter> getAllMeters();
 
@@ -93,7 +94,7 @@
      * precise device.
      *
      * @param deviceId the device to get the meter list from
-     * @return a collection of meters
+     * @return an immutable copy of the meters stored for a given device
      */
     Collection<Meter> getAllMeters(DeviceId deviceId);
 
diff --git a/core/api/src/test/java/org/onosproject/store/service/TestConsistentMap.java b/core/api/src/test/java/org/onosproject/store/service/TestConsistentMap.java
index 84d1734..e0df8c6 100644
--- a/core/api/src/test/java/org/onosproject/store/service/TestConsistentMap.java
+++ b/core/api/src/test/java/org/onosproject/store/service/TestConsistentMap.java
@@ -44,6 +44,8 @@
     private final String mapName;
     private final AtomicLong counter = new AtomicLong(0);
     private final Serializer serializer;
+    private Map<K, V> javaMap;
+
 
     private TestConsistentMap(String mapName, Serializer serializer) {
         map = new ConcurrentHashMap<>();
@@ -197,9 +199,7 @@
 
     @Override
     public Collection<Versioned<V>> values() {
-        return map.values()
-                .stream()
-                .collect(Collectors.toList());
+        return map.values();
     }
 
     @Override
@@ -295,7 +295,12 @@
 
     @Override
     public Map<K, V> asJavaMap() {
-        return new ConsistentMapBackedJavaMap<>(this);
+        synchronized (this) {
+            if (javaMap == null) {
+                javaMap = new ConsistentMapBackedJavaMap<>(this);
+            }
+        }
+        return javaMap;
     }
 
     public static Builder builder() {
diff --git a/core/net/src/main/java/org/onosproject/net/meter/impl/MeterManager.java b/core/net/src/main/java/org/onosproject/net/meter/impl/MeterManager.java
index d157260..a06a037 100644
--- a/core/net/src/main/java/org/onosproject/net/meter/impl/MeterManager.java
+++ b/core/net/src/main/java/org/onosproject/net/meter/impl/MeterManager.java
@@ -15,6 +15,7 @@
  */
 package org.onosproject.net.meter.impl;
 
+import com.google.common.collect.Sets;
 import org.onlab.util.PredictableExecutor;
 import org.onlab.util.PredictableExecutor.PickyRunnable;
 import org.onlab.util.Tools;
@@ -385,17 +386,26 @@
                     });
 
             // Update the meter stats in the store (first time move the state from pending to added)
+            Collection<Meter> addedMeters = Sets.newHashSet();
             meterEntries.stream()
                     .filter(m -> allMeters.stream()
                             .anyMatch(sm -> sm.deviceId().equals(deviceId) && sm.id().equals(m.id())))
-                    .forEach(m -> store.updateMeterState(m));
+                    .forEach(m -> {
+                        Meter updatedMeter = store.updateMeterState(m);
+                        if (updatedMeter != null && updatedMeter.state() == MeterState.ADDED) {
+                            addedMeters.add(updatedMeter);
+                        }
+                    });
+            Collection<Meter> newAllMeters = Sets.newHashSet(allMeters);
+            newAllMeters.removeAll(addedMeters);
 
-            allMeters.forEach(m -> {
+            newAllMeters.forEach(m -> {
                 // FIXME: Installing a meter is meaningful for OpenFlow, but not for P4Runtime.
                 // It looks like this flow is used only for p4runtime to emulate the installation
                 // since meters are already instantiated - we need just modify the params.
                 if (m.state() == MeterState.PENDING_ADD && m.meterCellId().type() != MeterCellType.INDEX) {
                     // offload the task to avoid the overloading of the sb threads
+                    log.debug("Modify meter {} in device {}", m.id(), deviceId);
                     meterInstallers.execute(new MeterInstaller(m.deviceId(), m, MeterOperation.Type.MODIFY));
                 // Remove workflow. Regarding OpenFlow, meters have been removed from
                 // the device but they are still in the store, we will purge them definitely.
@@ -403,6 +413,7 @@
                 // for P4Runtime will avoid to send a remove op. Then, we reach this point
                 // and we purge the meter from the store
                 } else if (m.state() == MeterState.PENDING_REMOVE) {
+                    log.debug("Delete meter {} now in store", m.id());
                     store.deleteMeterNow(m);
                 }
             });
diff --git a/core/store/dist/src/main/java/org/onosproject/store/meter/impl/DistributedMeterStore.java b/core/store/dist/src/main/java/org/onosproject/store/meter/impl/DistributedMeterStore.java
index 9aec726..11570f7 100644
--- a/core/store/dist/src/main/java/org/onosproject/store/meter/impl/DistributedMeterStore.java
+++ b/core/store/dist/src/main/java/org/onosproject/store/meter/impl/DistributedMeterStore.java
@@ -16,6 +16,7 @@
 package org.onosproject.store.meter.impl;
 
 import com.google.common.collect.Collections2;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
@@ -88,6 +89,7 @@
     private static final String METERSTORE = "onos-meter-store";
     private ConsistentMap<MeterKey, MeterData> meters;
     private MapEventListener<MeterKey, MeterData> mapListener = new InternalMapEventListener();
+    private Map<MeterKey, MeterData> metersMap;
 
     // Meters features related objects
     private static final String METERFEATURESSTORE = "onos-meter-features-store";
@@ -157,6 +159,7 @@
                                                  Meter.Unit.class,
                                                  MeterFailReason.class,
                                                  MeterFeaturesFlag.class)).build();
+        metersMap = meters.asJavaMap();
         // Init the set of the available ids
         availableMeterIds = new DefaultDistributedSet<>(storageService.<MeterKey>setBuilder()
                 .withName(AVAILABLEMETERIDSTORE)
@@ -276,10 +279,10 @@
     }
 
     @Override
-    public void updateMeterState(Meter meter) {
+    public Meter updateMeterState(Meter meter) {
         // Update meter if present (stats workflow)
         MeterKey key = MeterKey.key(meter.deviceId(), meter.id());
-        meters.computeIfPresent(key, (k, v) -> {
+        Versioned<MeterData> value = meters.computeIfPresent(key, (k, v) -> {
             DefaultMeter m = (DefaultMeter) v.meter();
             MeterState meterState = m.state();
             if (meterState == MeterState.PENDING_ADD) {
@@ -292,6 +295,7 @@
             m.setReferenceCount(meter.referenceCount());
             return new MeterData(m, null);
         });
+        return value != null ? value.value().meter() : null;
     }
 
     @Override
@@ -302,14 +306,14 @@
 
     @Override
     public Collection<Meter> getAllMeters() {
-        return Collections2.transform(meters.asJavaMap().values(),
+        return Collections2.transform(ImmutableSet.copyOf(metersMap.values()),
                                       MeterData::meter);
     }
 
     @Override
     public Collection<Meter> getAllMeters(DeviceId deviceId) {
         return Collections2.transform(
-                Collections2.filter(meters.asJavaMap().values(),
+                Collections2.filter(ImmutableSet.copyOf(metersMap.values()),
                         (MeterData m) -> m.meter().deviceId().equals(deviceId)),
                 MeterData::meter);
     }
diff --git a/core/store/dist/src/test/java/org/onosproject/store/meter/impl/DistributedMeterStoreTest.java b/core/store/dist/src/test/java/org/onosproject/store/meter/impl/DistributedMeterStoreTest.java
index 2566803..4cddf54 100644
--- a/core/store/dist/src/test/java/org/onosproject/store/meter/impl/DistributedMeterStoreTest.java
+++ b/core/store/dist/src/test/java/org/onosproject/store/meter/impl/DistributedMeterStoreTest.java
@@ -45,6 +45,7 @@
 import org.onosproject.store.service.Serializer;
 import org.onosproject.store.service.TestStorageService;
 
+import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.concurrent.CompletableFuture;
@@ -415,6 +416,64 @@
         assertNull(meterStore.getMeter(keyOne));
     }
 
+    /**
+     * Test getMeters API immutability.
+     */
+    @Test
+    public void testGetMetersImmutability() {
+        // Init the store
+        initMeterStore();
+
+        // Simulate the allocation of an id
+        MeterId idOne = meterStore.allocateMeterId(did1);
+        // Verify the allocation
+        assertThat(mid1, is(idOne));
+        // Let's create a meter
+        Meter meterOne = DefaultMeter.builder()
+                .forDevice(did1)
+                .fromApp(APP_ID)
+                .withId(mid1)
+                .withUnit(Meter.Unit.KB_PER_SEC)
+                .withBands(Collections.singletonList(b1))
+                .build();
+        // Set the state
+        ((DefaultMeter) meterOne).setState(MeterState.PENDING_ADD);
+        // Store the meter
+        meterStore.storeMeter(meterOne);
+
+        // Verify the immutability
+        Collection<Meter> meters = meterStore.getAllMeters();
+        Collection<Meter> metersDevice = meterStore.getAllMeters(did1);
+        assertThat(1, is(meters.size()));
+        assertThat(1, is(metersDevice.size()));
+
+        MeterId idTwo = meterStore.allocateMeterId(did1);
+        // Verify the allocation
+        assertThat(mid2, is(idTwo));
+        // Let's create a meter
+        Meter meterTwo = DefaultMeter.builder()
+                .forDevice(did1)
+                .fromApp(APP_ID)
+                .withId(mid2)
+                .withUnit(Meter.Unit.KB_PER_SEC)
+                .withBands(Collections.singletonList(b1))
+                .build();
+        // Set the state
+        ((DefaultMeter) meterTwo).setState(MeterState.PENDING_ADD);
+        // Store the meter
+        meterStore.storeMeter(meterTwo);
+
+        assertThat(1, is(meters.size()));
+        assertThat(1, is(metersDevice.size()));
+
+        meters = meterStore.getAllMeters();
+        metersDevice = meterStore.getAllMeters(did1);
+        assertThat(2, is(meters.size()));
+        assertThat(2, is(metersDevice.size()));
+
+
+    }
+
     // Test class for driver service.
     private class TestDriverService extends DriverServiceAdapter {
         @Override