Further cleanup of EffectiveRibInWriter

- prevent a potential NPE when empty change is received
- do not store modified children needlessly
- do not log delete duplicates
- do not mask optional to null
- separate out delete/modify/write table methods

Change-Id: I01b340fb3cb3ec0577ccd3856b79388115ff78df
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/EffectiveRibInWriter.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/EffectiveRibInWriter.java
index ccd88022d6e2970900c2e53721632e036cd62f27..b657428940350a16de11268808ee22b85f5c38ad 100644 (file)
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/EffectiveRibInWriter.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/EffectiveRibInWriter.java
@@ -179,19 +179,21 @@ final class EffectiveRibInWriter implements PrefixesReceivedCounters, PrefixesIn
             }
         }
 
-        final FluentFuture<? extends CommitInfo> future = tx.commit();
-        this.submitted = future;
-        future.addCallback(new FutureCallback<CommitInfo>() {
-            @Override
-            public void onSuccess(final CommitInfo result) {
-                LOG.trace("Successful commit");
-            }
+        if (tx != null) {
+            final FluentFuture<? extends CommitInfo> future = tx.commit();
+            this.submitted = future;
+            future.addCallback(new FutureCallback<CommitInfo>() {
+                @Override
+                public void onSuccess(final CommitInfo result) {
+                    LOG.trace("Successful commit");
+                }
 
-            @Override
-            public void onFailure(final Throwable trw) {
-                LOG.error("Failed commit", trw);
-            }
-        }, MoreExecutors.directExecutor());
+                @Override
+                public void onFailure(final Throwable trw) {
+                    LOG.error("Failed commit", trw);
+                }
+            }, MoreExecutors.directExecutor());
+        }
 
         //Refresh VPN Table if RT Memberships were updated
         if (this.rtMembershipsUpdated) {
@@ -214,24 +216,20 @@ final class EffectiveRibInWriter implements PrefixesReceivedCounters, PrefixesIn
             return;
         }
 
-        final RIBSupport<?, ?, ?, ?> ribSupport = ribContext.getRibSupport();
         final YangInstanceIdentifier effectiveTablePath = effectiveTablePath(tableKey);
         final ModificationType modificationType = root.getModificationType();
+        LOG.debug("Effective table {} modification type {}", effectiveTablePath, modificationType);
         switch (modificationType) {
             case DISAPPEARED:
             case DELETE:
-                processTableChildren(tx, ribSupport, effectiveTablePath, table.getChildNodes());
-                LOG.debug("Delete Effective Table {} modification type {}, ", effectiveTablePath, modificationType);
-                tx.delete(LogicalDatastoreType.OPERATIONAL, effectiveTablePath);
+                deleteTable(tx, ribContext, effectiveTablePath, table);
                 break;
             case APPEARED:
             case WRITE:
-                LOG.trace("Create Empty table {}", ribSupport.getTablesKey());
-                ribContext.createEmptyTableStructure(tx, effectiveTablePath);
-                processTableChildren(tx, ribSupport, effectiveTablePath, table.getChildNodes());
+                writeTable(tx, ribContext, effectiveTablePath, table);
                 break;
             case SUBTREE_MODIFIED:
-                processTableChildren(tx, ribSupport, effectiveTablePath, table.getChildNodes());
+                modifyTable(tx, ribContext, effectiveTablePath, table);
                 break;
             case UNMODIFIED:
                 LOG.info("Ignoring spurious notification on {} data {}", rootPath, table);
@@ -242,21 +240,38 @@ final class EffectiveRibInWriter implements PrefixesReceivedCounters, PrefixesIn
         }
     }
 
+    private void deleteTable(final DOMDataWriteTransaction tx, final RIBSupportContext ribContext,
+            final YangInstanceIdentifier effectiveTablePath, final DataTreeCandidateNode table) {
+        processTableChildren(tx, ribContext.getRibSupport(), effectiveTablePath, table.getChildNodes());
+        LOG.debug("Delete Effective Table {}", effectiveTablePath);
+        tx.delete(LogicalDatastoreType.OPERATIONAL, effectiveTablePath);
+    }
+
+    private void modifyTable(final DOMDataWriteTransaction tx, final RIBSupportContext ribContext,
+            final YangInstanceIdentifier effectiveTablePath, final DataTreeCandidateNode table) {
+        LOG.debug("Modify Effective Table {}", effectiveTablePath);
+        processTableChildren(tx, ribContext.getRibSupport(), effectiveTablePath, table.getChildNodes());
+    }
+
+    private void writeTable(final DOMDataWriteTransaction tx, final RIBSupportContext ribContext,
+            final YangInstanceIdentifier effectiveTablePath, final DataTreeCandidateNode table) {
+        LOG.debug("Write Effective Table {}", effectiveTablePath);
+        ribContext.createEmptyTableStructure(tx, effectiveTablePath);
+        processTableChildren(tx, ribContext.getRibSupport(), effectiveTablePath, table.getChildNodes());
+    }
+
     private void processTableChildren(final DOMDataWriteTransaction tx, final RIBSupport<?, ?, ?, ?> ribSupport,
             final YangInstanceIdentifier effectiveTablePath, final Collection<DataTreeCandidateNode> children) {
         for (final DataTreeCandidateNode child : children) {
             final PathArgument childIdentifier = child.getIdentifier();
+            final ModificationType childModType = child.getModificationType();
             final Optional<NormalizedNode<?, ?>> childDataAfter = child.getDataAfter();
-            LOG.debug("Process table {} type {}, dataAfter {}, dataBefore {}", childIdentifier, child
-                .getModificationType(), childDataAfter, child.getDataBefore());
+            LOG.debug("Process table {} type {}, dataAfter {}, dataBefore {}", childIdentifier, childModType,
+                childDataAfter, child.getDataBefore());
             final YangInstanceIdentifier routesPath = effectiveTablePath.node(childIdentifier);
-            switch (child.getModificationType()) {
+            switch (childModType) {
                 case DELETE:
-                    LOG.debug("Route deleted. routeId={}", routesPath);
-                    processTableChildrenDelete(child, childIdentifier, tx, ribSupport, routesPath);
-                    break;
                 case DISAPPEARED:
-                    LOG.debug("Route disappeared. routeId={}", routesPath);
                     processTableChildrenDelete(child, childIdentifier, tx, ribSupport, routesPath);
                     break;
                 case UNMODIFIED:
@@ -280,16 +295,13 @@ final class EffectiveRibInWriter implements PrefixesReceivedCounters, PrefixesIn
             final DOMDataWriteTransaction tx, final RIBSupport<?, ?, ?, ?> ribSupport,
             final YangInstanceIdentifier routesPath) {
         if (ROUTES.equals(childIdentifier)) {
-            final Collection<DataTreeCandidateNode> changedRoutes = ribSupport.changedRoutes(child);
-            if (!changedRoutes.isEmpty()) {
-                for (final DataTreeCandidateNode route : changedRoutes) {
-                    final NormalizedNode<?, ?> routeBefore = route.getDataBefore().orElse(null);
-                    if (routeBefore != null) {
-                        final YangInstanceIdentifier routePath = ribSupport.routePath(routesPath, route.getIdentifier());
-                        handleRouteTarget(ModificationType.DELETE, ribSupport, routePath, routeBefore);
-                        final TablesKey tablesKey = ribSupport.getTablesKey();
-                        CountersUtil.decrement(this.prefixesInstalled.get(tablesKey), tablesKey);
-                    }
+            for (final DataTreeCandidateNode route : ribSupport.changedRoutes(child)) {
+                final Optional<NormalizedNode<?, ?>> routeBefore = route.getDataBefore();
+                if (routeBefore.isPresent()) {
+                    final YangInstanceIdentifier routePath = ribSupport.routePath(routesPath, route.getIdentifier());
+                    handleRouteTarget(ModificationType.DELETE, ribSupport, routePath, routeBefore.get());
+                    final TablesKey tablesKey = ribSupport.getTablesKey();
+                    CountersUtil.decrement(this.prefixesInstalled.get(tablesKey), tablesKey);
                 }
             }
         }
@@ -300,11 +312,8 @@ final class EffectiveRibInWriter implements PrefixesReceivedCounters, PrefixesIn
             final DOMDataWriteTransaction tx, final RIBSupport<?, ?, ?, ?> ribSupport,
             final YangInstanceIdentifier routesPath, final Optional<NormalizedNode<?, ?>> childDataAfter) {
         if (ROUTES.equals(childIdentifier)) {
-            final Collection<DataTreeCandidateNode> changedRoutes = ribSupport.changedRoutes(child);
-            if (!changedRoutes.isEmpty()) {
-                for (final DataTreeCandidateNode route : changedRoutes) {
-                    processRoute(tx, ribSupport, routesPath.getParent(), route);
-                }
+            for (final DataTreeCandidateNode route : ribSupport.changedRoutes(child)) {
+                processRoute(tx, ribSupport, routesPath.getParent(), route);
             }
         } else {
             tx.put(LogicalDatastoreType.OPERATIONAL, routesPath, childDataAfter.get());