Use ResourceId when unregistering resources instead of Resource
Change-Id: Ib3d1c611ad9eb4491693ea10ee50ea0123f20ee2
diff --git a/core/api/src/main/java/org/onosproject/net/newresource/ResourceAdminService.java b/core/api/src/main/java/org/onosproject/net/newresource/ResourceAdminService.java
index d482983..23f79e6 100644
--- a/core/api/src/main/java/org/onosproject/net/newresource/ResourceAdminService.java
+++ b/core/api/src/main/java/org/onosproject/net/newresource/ResourceAdminService.java
@@ -48,22 +48,21 @@
/**
* Unregisters the specified resources.
*
- * @param resources resources to be unregistered
+ * @param ids IDs of resources to be unregistered
* @return true if unregistration is successfully done, false otherwise. Unregistration
* succeeds when each resource is not registered or unallocated.
*/
- // TODO: might need to change the first argument type to ResourceId
- default boolean unregisterResources(Resource... resources) {
- return unregisterResources(ImmutableList.copyOf(resources));
+ default boolean unregisterResources(ResourceId... ids) {
+ return unregisterResources(ImmutableList.copyOf(ids));
}
/**
* Unregisters the specified resources.
*
- * @param resources resources to be unregistered
+ * @param ids IDs of resources to be unregistered
* @return true if unregistration is successfully done, false otherwise. Unregistration
* succeeds when each resource is not registered or unallocated.
*/
// TODO: might need to change the first argument type to ResourceId
- boolean unregisterResources(List<Resource> resources);
+ boolean unregisterResources(List<ResourceId> ids);
}
diff --git a/core/api/src/main/java/org/onosproject/net/newresource/ResourceStore.java b/core/api/src/main/java/org/onosproject/net/newresource/ResourceStore.java
index 0e3eee8..aee72f1 100644
--- a/core/api/src/main/java/org/onosproject/net/newresource/ResourceStore.java
+++ b/core/api/src/main/java/org/onosproject/net/newresource/ResourceStore.java
@@ -45,10 +45,10 @@
* or none of the given resources is unregistered. The whole unregistration fails when any one of the
* resource can't be unregistered.
*
- * @param resources resources to be unregistered
+ * @param ids resources to be unregistered
* @return true if the registration succeeds, false otherwise
*/
- boolean unregister(List<Resource> resources);
+ boolean unregister(List<ResourceId> ids);
/**
* Allocates the specified resources to the specified consumer in transactional way.
diff --git a/core/net/src/main/java/org/onosproject/net/newresource/impl/ResourceDeviceListener.java b/core/net/src/main/java/org/onosproject/net/newresource/impl/ResourceDeviceListener.java
index a8850df..0c94ab5 100644
--- a/core/net/src/main/java/org/onosproject/net/newresource/impl/ResourceDeviceListener.java
+++ b/core/net/src/main/java/org/onosproject/net/newresource/impl/ResourceDeviceListener.java
@@ -16,6 +16,7 @@
package org.onosproject.net.newresource.impl;
import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Lists;
import org.onlab.packet.MplsLabel;
import org.onlab.packet.VlanId;
import org.onlab.util.Bandwidth;
@@ -133,7 +134,7 @@
executor.submit(() -> {
DiscreteResource devResource = Resources.discrete(device.id()).resource();
List<Resource> allResources = getDescendantResources(devResource);
- adminService.unregisterResources(allResources);
+ adminService.unregisterResources(Lists.transform(allResources, Resource::id));
});
}
@@ -189,7 +190,7 @@
executor.submit(() -> {
DiscreteResource portResource = Resources.discrete(device.id(), port.number()).resource();
List<Resource> allResources = getDescendantResources(portResource);
- adminService.unregisterResources(allResources);
+ adminService.unregisterResources(Lists.transform(allResources, Resource::id));
});
}
diff --git a/core/net/src/main/java/org/onosproject/net/newresource/impl/ResourceManager.java b/core/net/src/main/java/org/onosproject/net/newresource/impl/ResourceManager.java
index 5fc9d55..7ce8c28 100644
--- a/core/net/src/main/java/org/onosproject/net/newresource/impl/ResourceManager.java
+++ b/core/net/src/main/java/org/onosproject/net/newresource/impl/ResourceManager.java
@@ -178,10 +178,10 @@
}
@Override
- public boolean unregisterResources(List<Resource> resources) {
- checkNotNull(resources);
+ public boolean unregisterResources(List<ResourceId> ids) {
+ checkNotNull(ids);
- return store.unregister(resources);
+ return store.unregister(ids);
}
private class InternalStoreDelegate implements ResourceStoreDelegate {
diff --git a/core/net/src/main/java/org/onosproject/net/newresource/impl/ResourceNetworkConfigListener.java b/core/net/src/main/java/org/onosproject/net/newresource/impl/ResourceNetworkConfigListener.java
index e98a3c3..39ab964 100644
--- a/core/net/src/main/java/org/onosproject/net/newresource/impl/ResourceNetworkConfigListener.java
+++ b/core/net/src/main/java/org/onosproject/net/newresource/impl/ResourceNetworkConfigListener.java
@@ -117,7 +117,7 @@
// FIXME Following should be an update to the value based on port speed
if (!adminService.unregisterResources(Resources.continuous(cp.deviceId(),
cp.port(),
- Bandwidth.class).resource(0))) {
+ Bandwidth.class).id())) {
log.warn("Failed to unregister Bandwidth for {}", cp);
}
break;
@@ -148,7 +148,7 @@
// returns true (success)
if (!adminService.unregisterResources(
- Resources.continuous(cp.deviceId(), cp.port(), Bandwidth.class).resource(0))) {
+ Resources.continuous(cp.deviceId(), cp.port(), Bandwidth.class).id())) {
log.warn("unregisterResources for {} failed", cp);
}
return adminService.registerResources(Resources.continuous(cp.deviceId(),
diff --git a/core/store/dist/src/main/java/org/onosproject/store/newresource/impl/ConsistentResourceStore.java b/core/store/dist/src/main/java/org/onosproject/store/newresource/impl/ConsistentResourceStore.java
index 1d89e11..173851d 100644
--- a/core/store/dist/src/main/java/org/onosproject/store/newresource/impl/ConsistentResourceStore.java
+++ b/core/store/dist/src/main/java/org/onosproject/store/newresource/impl/ConsistentResourceStore.java
@@ -167,8 +167,7 @@
.collect(Collectors.groupingBy(x -> x.parent().get()));
for (Map.Entry<DiscreteResource, List<Resource>> entry: resourceMap.entrySet()) {
- Optional<DiscreteResource> child = lookup(childTxMap, entry.getKey());
- if (!child.isPresent()) {
+ if (!lookup(childTxMap, entry.getKey().id()).isPresent()) {
return abortTransaction(tx);
}
@@ -189,8 +188,8 @@
}
@Override
- public boolean unregister(List<Resource> resources) {
- checkNotNull(resources);
+ public boolean unregister(List<ResourceId> ids) {
+ checkNotNull(ids);
TransactionContext tx = service.transactionContextBuilder().build();
tx.begin();
@@ -203,8 +202,13 @@
tx.getTransactionalMap(CONTINUOUS_CONSUMER_MAP, SERIALIZER);
// Extract Discrete instances from resources
- Map<DiscreteResourceId, List<Resource>> resourceMap = resources.stream()
+ List<Resource> resources = ids.stream()
.filter(x -> x.parent().isPresent())
+ .map(x -> lookup(childTxMap, x))
+ .filter(Optional::isPresent)
+ .map(Optional::get)
+ .collect(Collectors.toList());
+ Map<DiscreteResourceId, List<Resource>> resourceMap = resources.stream()
.collect(Collectors.groupingBy(x -> x.parent().get().id()));
// even if one of the resources is allocated to a consumer,
@@ -241,7 +245,7 @@
.collect(Collectors.toList());
notifyDelegate(events);
} else {
- log.warn("Failed to unregister {}: Commit failed.", resources);
+ log.warn("Failed to unregister {}: Commit failed.", ids);
}
return success;
}
@@ -263,7 +267,7 @@
for (Resource resource: resources) {
if (resource instanceof DiscreteResource) {
- if (!lookup(childTxMap, resource).isPresent()) {
+ if (!lookup(childTxMap, resource.id()).isPresent()) {
return abortTransaction(tx);
}
@@ -272,18 +276,20 @@
return abortTransaction(tx);
}
} else if (resource instanceof ContinuousResource) {
- Optional<ContinuousResource> continuous = lookup(childTxMap, (ContinuousResource) resource);
- if (!continuous.isPresent()) {
+ Optional<Resource> lookedUp = lookup(childTxMap, resource.id());
+ if (!lookedUp.isPresent()) {
return abortTransaction(tx);
}
- ContinuousResourceAllocation allocations = continuousConsumerTxMap.get(continuous.get().id());
- if (!hasEnoughResource(continuous.get(), (ContinuousResource) resource, allocations)) {
+ // Down cast: this must be safe as ContinuousResource is associated with ContinuousResourceId
+ ContinuousResource continuous = (ContinuousResource) lookedUp.get();
+ ContinuousResourceAllocation allocations = continuousConsumerTxMap.get(continuous.id());
+ if (!hasEnoughResource(continuous, (ContinuousResource) resource, allocations)) {
return abortTransaction(tx);
}
boolean success = appendValue(continuousConsumerTxMap,
- continuous.get(), new ResourceAllocation(continuous.get(), consumer));
+ continuous, new ResourceAllocation(continuous, consumer));
if (!success) {
return abortTransaction(tx);
}
@@ -534,34 +540,29 @@
}
/**
- * Returns the resource which has the same key as the key of the specified resource
- * in the list as a value of the map.
+ * Returns the resource which has the same key as the specified resource ID
+ * in the set as a value of the map.
*
- * @param map map storing parent - child relationship of resources
- * @param resource resource to be checked for its key
+ * @param childTxMap map storing parent - child relationship of resources
+ * @param id ID of resource to be checked
* @return the resource which is regarded as the same as the specified resource
*/
- // Naive implementation, which traverses all elements in the list
+ // Naive implementation, which traverses all elements in the set
// computational complexity: O(n) where n is the number of elements
// in the associated set
- private <T extends Resource> Optional<T> lookup(
- TransactionalMap<DiscreteResourceId, Set<Resource>> map, T resource) {
- // if it is root, always returns itself
- if (!resource.parent().isPresent()) {
- return Optional.of(resource);
+ private Optional<Resource> lookup(TransactionalMap<DiscreteResourceId, Set<Resource>> childTxMap, ResourceId id) {
+ if (!id.parent().isPresent()) {
+ return Optional.of(Resource.ROOT);
}
- Set<Resource> values = map.get(resource.parent().get().id());
+ Set<Resource> values = childTxMap.get(id.parent().get());
if (values == null) {
return Optional.empty();
}
- @SuppressWarnings("unchecked")
- Optional<T> result = values.stream()
- .filter(x -> x.id().equals(resource.id()))
- .map(x -> (T) x)
+ return values.stream()
+ .filter(x -> x.id().equals(id))
.findFirst();
- return result;
}
/**