From 26dda3a9a7c63da4c071d6ad80bcc268cd781cc8 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Fri, 25 Jan 2019 19:01:53 +0100 Subject: [PATCH] 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 --- .../bgp/rib/impl/EffectiveRibInWriter.java | 93 ++++++++++--------- 1 file changed, 51 insertions(+), 42 deletions(-) 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 ccd88022d6..b657428940 100644 --- 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 future = tx.commit(); - this.submitted = future; - future.addCallback(new FutureCallback() { - @Override - public void onSuccess(final CommitInfo result) { - LOG.trace("Successful commit"); - } + if (tx != null) { + final FluentFuture future = tx.commit(); + this.submitted = future; + future.addCallback(new FutureCallback() { + @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 children) { for (final DataTreeCandidateNode child : children) { final PathArgument childIdentifier = child.getIdentifier(); + final ModificationType childModType = child.getModificationType(); final Optional> 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 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> 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> childDataAfter) { if (ROUTES.equals(childIdentifier)) { - final Collection 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()); -- 2.36.6