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);
+   }
+}