Distributed Dynamic Config Store fixes
- mostly cosmetic & logging
- fixes bug dealing with root DocumentPath conversion to String
- clarification and notes around dynamic config
Change-Id: I94284f591b4045461f5121ad8318e6aa5f35fd7c
diff --git a/apps/config/src/main/java/org/onosproject/config/DynamicConfigEvent.java b/apps/config/src/main/java/org/onosproject/config/DynamicConfigEvent.java
index a2e513e..1e4296a 100644
--- a/apps/config/src/main/java/org/onosproject/config/DynamicConfigEvent.java
+++ b/apps/config/src/main/java/org/onosproject/config/DynamicConfigEvent.java
@@ -67,4 +67,17 @@
public DynamicConfigEvent(Type type, ResourceId path) {
super(type, path);
}
-}
\ No newline at end of file
+
+ /**
+ * Returns ResourceId relative to root node.
+ * (=root will not be part of ResourceId returned)
+ *
+ * @return resource Id
+ */
+ @Override
+ public ResourceId subject() {
+ return super.subject();
+ }
+
+ // TODO add DataNode as event payload
+}
diff --git a/apps/config/src/main/java/org/onosproject/config/DynamicConfigService.java b/apps/config/src/main/java/org/onosproject/config/DynamicConfigService.java
index a5e881a..bb562e8 100644
--- a/apps/config/src/main/java/org/onosproject/config/DynamicConfigService.java
+++ b/apps/config/src/main/java/org/onosproject/config/DynamicConfigService.java
@@ -32,6 +32,12 @@
public interface DynamicConfigService
extends ListenerService<DynamicConfigEvent, DynamicConfigListener> {
+ // FIXME revisit and verify ResourceId documentation.
+ // it is likely that it actually is not expecting absolute ResourceId
+
+ // TODO revisit which path ResourceId these API should accepting.
+ // there is inconsistency, some expect parent, some expect node itself
+
/**
* Creates a new node in the dynamic config store.
* This method would throw an exception if there is a node with the same
@@ -39,11 +45,11 @@
* nodes were not present in the path leading up to the requested node.
* Failure reason will be the error message in the exception.
*
- * @param path data structure with absolute path to the parent
+ * @param parent data structure with absolute path to the parent
* @param node recursive data structure, holding a leaf node or a subtree
* @throws FailedException if the new node could not be created
*/
- void createNode(ResourceId path, DataNode node);
+ void createNode(ResourceId parent, DataNode node);
/**
* Reads the requested node form the dynamic config store.
@@ -76,11 +82,11 @@
* parent nodes in the path were not present.
* Failure reason will be the error message in the exception.
*
- * @param path data structure with absolute path to the parent
+ * @param parent data structure with absolute path to the parent
* @param node recursive data structure, holding a leaf node or a subtree
* @throws FailedException if the update request failed for any reason
*/
- void updateNode(ResourceId path, DataNode node);
+ void updateNode(ResourceId parent, DataNode node);
/**
* Replaces nodes in the dynamic config store.
@@ -89,11 +95,11 @@
* the requested node or any of the parent nodes in the path were not
* present. Failure reason will be the error message in the exception.
*
- * @param path data structure with absolute path to the parent
+ * @param parent data structure with absolute path to the parent
* @param node recursive data structure, holding a leaf node or a subtree
* @throws FailedException if the replace request failed
*/
- void replaceNode(ResourceId path, DataNode node);
+ void replaceNode(ResourceId parent, DataNode node);
/**
* Removes a node from the dynamic config store.
diff --git a/apps/config/src/main/java/org/onosproject/config/FailedException.java b/apps/config/src/main/java/org/onosproject/config/FailedException.java
index 0115c33..80559ed 100644
--- a/apps/config/src/main/java/org/onosproject/config/FailedException.java
+++ b/apps/config/src/main/java/org/onosproject/config/FailedException.java
@@ -23,6 +23,8 @@
@Beta
public class FailedException extends RuntimeException {
+ private static final long serialVersionUID = 266049754055616595L;
+
/**
* Constructs a new runtime exception with no error message.
*/
diff --git a/apps/config/src/main/java/org/onosproject/config/InvalidFilterException.java b/apps/config/src/main/java/org/onosproject/config/InvalidFilterException.java
index fcb73b0..1406b20 100644
--- a/apps/config/src/main/java/org/onosproject/config/InvalidFilterException.java
+++ b/apps/config/src/main/java/org/onosproject/config/InvalidFilterException.java
@@ -23,6 +23,8 @@
@Beta
public class InvalidFilterException extends RuntimeException {
+ private static final long serialVersionUID = 7270417891847061766L;
+
/**
* Constructs a new runtime exception with no error message.
*/
@@ -38,4 +40,6 @@
public InvalidFilterException(String message) {
super(message);
}
+
+ // TODO add constructor with cause
}
\ No newline at end of file
diff --git a/apps/config/src/main/java/org/onosproject/config/ResourceIdParser.java b/apps/config/src/main/java/org/onosproject/config/ResourceIdParser.java
index 0236657..dc93e58 100644
--- a/apps/config/src/main/java/org/onosproject/config/ResourceIdParser.java
+++ b/apps/config/src/main/java/org/onosproject/config/ResourceIdParser.java
@@ -26,6 +26,8 @@
import org.onosproject.yang.model.NodeKey;
import org.onosproject.yang.model.ResourceId;
+import com.google.common.annotations.Beta;
+
// FIXME add non-trivial examples
/**
* Utilities to work on the ResourceId.
@@ -38,6 +40,7 @@
*
*/
//FIXME add javadocs
+@Beta
public final class ResourceIdParser {
/**
@@ -129,6 +132,7 @@
}
public static String appendNodeKey(String path, NodeKey key) {
+ // FIXME this is not handling root path exception
return (path + EL_SEP + key.schemaId().name() + NM_SEP + key.schemaId().namespace());
}
@@ -199,7 +203,8 @@
while (itr.hasNext()) {
nodeKeyList.add(itr.next());
}
- if (nodeKeyList.get(0).schemaId().name().compareTo("/") == 0) {
+ // exception for dealing with root
+ if (nodeKeyList.get(0).schemaId().name().equals("/")) {
nodeKeyList.remove(0);
}
for (NodeKey key : nodeKeyList) {
diff --git a/apps/config/src/main/java/org/onosproject/config/impl/DistributedDynamicConfigStore.java b/apps/config/src/main/java/org/onosproject/config/impl/DistributedDynamicConfigStore.java
index 7c73922..97731a6 100644
--- a/apps/config/src/main/java/org/onosproject/config/impl/DistributedDynamicConfigStore.java
+++ b/apps/config/src/main/java/org/onosproject/config/impl/DistributedDynamicConfigStore.java
@@ -29,6 +29,7 @@
import org.onosproject.config.FailedException;
import org.onosproject.config.Filter;
import org.onosproject.config.ResourceIdParser;
+import org.onosproject.d.config.ResourceIds;
import org.onosproject.store.AbstractStore;
import org.onosproject.store.serializers.KryoNamespaces;
import org.onosproject.store.service.AsyncDocumentTree;
@@ -57,7 +58,9 @@
import org.slf4j.LoggerFactory;
import java.math.BigInteger;
+import java.util.LinkedHashMap;
import java.util.Map;
+import java.util.Optional;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutionException;
@@ -75,11 +78,19 @@
public class DistributedDynamicConfigStore
extends AbstractStore<DynamicConfigEvent, DynamicConfigStoreDelegate>
implements DynamicConfigStore {
+
private final Logger log = LoggerFactory.getLogger(getClass());
+
@Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
protected StorageService storageService;
+
+ // FIXME transactionally mutate the 2 or consolidate into 1 AsyncDocTree
+ // effectively tree structure only
private AsyncDocumentTree<DataNode.Type> keystore;
+ // TODO Can we pass DocumentPath directly to ConsistentMap?
+ // Map<DocumentPath as String, leaf value>
private ConsistentMap<String, LeafNode> objectStore;
+
private final DocumentTreeListener<DataNode.Type> klistener = new InternalDocTreeListener();
private final MapEventListener<String, LeafNode> olistener = new InternalMapListener();
@@ -87,7 +98,7 @@
public void activateStore() {
KryoNamespace.Builder kryoBuilder = new KryoNamespace.Builder()
.register(KryoNamespaces.BASIC)
- .register(java.lang.Class.class)
+ .register(Class.class)
.register(DataNode.Type.class)
.register(LeafNode.class)
.register(InnerNode.class)
@@ -98,7 +109,7 @@
.register(ListKey.class)
.register(KeyLeaf.class)
.register(BigInteger.class)
- .register(java.util.LinkedHashMap.class);
+ .register(LinkedHashMap.class);
keystore = storageService.<DataNode.Type>documentTreeBuilder()
.withSerializer(Serializer.using(kryoBuilder.build()))
.withName("config-key-store")
@@ -124,8 +135,10 @@
@Override
public CompletableFuture<Boolean>
- addNode(ResourceId complete, DataNode node) {
- String spath = ResourceIdParser.parseResId(complete);
+ addNode(ResourceId parent, DataNode node) {
+ String spath = ResourceIdParser.parseResId(parent);
+ log.trace(" addNode({}, {})", parent, node);
+ log.trace(" spath={}", spath);
if (spath == null) {
throw new FailedException("Invalid RsourceId, cannot create Node");
}
@@ -134,12 +147,19 @@
throw new FailedException("Node or parent does not exist for " + spath);
}
}
- spath = ResourceIdParser.appendNodeKey(spath, node.key());
- parseNode(spath, node);
+ ResourceId abs = ResourceIds.resourceId(parent, node);
+ //spath = ResourceIdParser.appendNodeKey(spath, node.key());
+ parseNode(ResourceIdParser.parseResId(abs), node);
return CompletableFuture.completedFuture(true);
}
+ // FIXME this is more like addNode
+ /**
+ * @param path pointing to {@code node}
+ * @param node node
+ */
private void parseNode(String path, DataNode node) {
+ log.trace("parseNode({}, {})", path, node);
if (completeVersioned(keystore.get(DocumentPath.from(path))) != null) {
throw new FailedException("Requested node already present in the" +
" store, please use an update method");
@@ -167,12 +187,19 @@
}
}
+ // FIXME this is more like addInnteNode
+ /**
+ * @param path pointing to {@code node}
+ * @param node node
+ */
private void traverseInner(String path, InnerNode node) {
+ log.trace("traverseInner({}, {})", path, node);
addKey(path, node.type());
Map<NodeKey, DataNode> entries = node.childNodes();
if (entries.size() == 0) {
return;
}
+ // FIXME ignoring results
entries.forEach((k, v) -> {
String tempPath;
tempPath = ResourceIdParser.appendNodeKey(path, v.key());
@@ -198,11 +225,19 @@
}
private Boolean addKey(String path, DataNode.Type type) {
- if (completeVersioned(keystore.get(DocumentPath.from(path))) != null) {
- completeVersioned(keystore.set(DocumentPath.from(path), type));
+ log.trace("addKey({}, {})", path, type);
+ DocumentPath dpath = DocumentPath.from(path);
+ log.trace("dpath={}", dpath);
+ // FIXME Not atomic, should probably use create or replace
+ if (completeVersioned(keystore.get(dpath)) != null) {
+ completeVersioned(keystore.set(dpath, type));
+ log.trace("true");
return true;
}
- return complete(keystore.create(DocumentPath.from(path), type));
+ log.trace(" keystore.create({}, {})", dpath, type);
+ Boolean result = complete(keystore.create(dpath, type));
+ log.trace("{}", result);
+ return result;
}
@Override
@@ -489,10 +524,10 @@
throw new FailedException(e.getCause().getMessage());
} catch (ExecutionException e) {
if (e.getCause() instanceof IllegalDocumentModificationException) {
- throw new FailedException("Node or parent doesnot exist or is root or is not a Leaf Node",
+ throw new FailedException("Node or parent does not exist or is root or is not a Leaf Node",
e.getCause());
} else if (e.getCause() instanceof NoSuchDocumentPathException) {
- throw new FailedException("Resource id does not exist", e.getCause());
+ throw new FailedException("ResourceId does not exist", e.getCause());
} else {
throw new FailedException("Datastore operation failed", e.getCause());
}
@@ -500,25 +535,8 @@
}
private <T> T completeVersioned(CompletableFuture<Versioned<T>> future) {
- try {
- if (future.get() != null) {
- return future.get().value();
- } else {
- return null;
- }
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- throw new FailedException(e.getCause().getMessage());
- } catch (ExecutionException e) {
- if (e == null) {
- throw new FailedException("Unknown Exception");
- } else if (e.getCause() instanceof IllegalDocumentModificationException) {
- throw new FailedException("Node or parent does not exist or is root or is not a Leaf Node");
- } else if (e.getCause() instanceof NoSuchDocumentPathException) {
- throw new FailedException("Resource id does not exist");
- } else {
- throw new FailedException("Datastore operation failed");
- }
- }
- }
-}
\ No newline at end of file
+ return Optional.ofNullable(complete(future))
+ .map(Versioned::value)
+ .orElse(null);
+ }
+}