Refactor AbstractPeer initialization

We are running createDomChain() in constructor, which not only triggers
SpotBugs, but also results in bad logging: 'Creating DOM peer chain null'
because peerId is not initialized yet.

Fix this by moving the call to createDomChain() to subclasses.

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

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractPeer.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractPeer.java
index 96ef6019618fc6b59989f499feb138f8b6214200..02b9bbfeaf83653f1e85ebd8465288ee380927bd 100644 (file)
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractPeer.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractPeer.java
@@ -13,8 +13,6 @@ import com.google.common.annotations.VisibleForTesting;
 import com.google.common.util.concurrent.FluentFuture;
 import com.google.common.util.concurrent.FutureCallback;
 import com.google.common.util.concurrent.MoreExecutors;
-import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
-import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -84,7 +82,7 @@ abstract class AbstractPeer extends BGPPeerStateImpl implements BGPRouteEntryImp
     //        hence we want to use the two chains concurrently. The problem is their lifecycle in response to errors,
     //        which needs figuring out.
     @GuardedBy("this")
-    private DOMTransactionChain domChain;
+    private DOMTransactionChain domChain = null;
     // FIXME: This is an invariant once the peer is 'resolved' -- which happens instantaneously for ApplicationPeer.
     //        There are also a number YangInstanceIdentifiers which are tied to it. We want to keep all of them in one
     //        structure for isolation. This could be a separate DTO (JDK16 record) or isolated into an abstract behavior
@@ -99,8 +97,6 @@ abstract class AbstractPeer extends BGPPeerStateImpl implements BGPRouteEntryImp
     @GuardedBy("this")
     private FluentFuture<? extends CommitInfo> submitted;
 
-    @SuppressFBWarnings(value = "MC_OVERRIDABLE_METHOD_CALL_IN_CONSTRUCTOR",
-        justification = "False positive on synchronized createDomChain()")
     AbstractPeer(
             final RIB rib,
             final String peerName,
@@ -119,18 +115,6 @@ abstract class AbstractPeer extends BGPPeerStateImpl implements BGPRouteEntryImp
         this.clusterId = clusterId;
         this.localAs = localAs;
         this.rib = rib;
-        createDomChain();
-    }
-
-    AbstractPeer(
-            final RIB rib,
-            final String peerName,
-            final String groupId,
-            final PeerRole role,
-            final IpAddressNoZone neighborAddress,
-            final Set<TablesKey> afiSafisGracefulAdvertized) {
-        this(rib, peerName, groupId, role, null, null, neighborAddress,
-                rib.getLocalTablesKeys(), afiSafisGracefulAdvertized, Collections.emptyMap());
     }
 
     final synchronized FluentFuture<? extends CommitInfo> removePeer(final @Nullable YangInstanceIdentifier peerPath) {

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ApplicationPeer.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ApplicationPeer.java
index f55b10a5ddd3185badbc70502e9cd2636980a98c..1c530731be163f091ac7f0d03ffc61d2172e8ef8 100644 (file)
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ApplicationPeer.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/ApplicationPeer.java
@@ -76,6 +76,14 @@ import org.slf4j.LoggerFactory;
  * peer needs to have a BGP-ID that is configurable.
  */
 public class ApplicationPeer extends AbstractPeer implements DOMDataTreeChangeListener {
+    @FunctionalInterface
+    interface RegisterAppPeerListener {
+        /**
+         * Register Application Peer Change Listener once AdjRibIn has been successfully initialized.
+         */
+        void register();
+    }
+
     private static final Logger LOG = LoggerFactory.getLogger(ApplicationPeer.class);
     private static final String APP_PEER_GROUP = "application-peers";
 
@@ -98,32 +106,19 @@ public class ApplicationPeer extends AbstractPeer implements DOMDataTreeChangeLi
     private Registration trackerRegistration;
     private YangInstanceIdentifier peerPath;
 
-    @Override
-    public List<RouteTarget> getMemberships() {
-        return List.of();
-    }
-
-    @FunctionalInterface
-    interface RegisterAppPeerListener {
-        /**
-         * Register Application Peer Change Listener once AdjRibIn has been successfully initialized.
-         */
-        void register();
-    }
-
     public ApplicationPeer(
             final BGPTableTypeRegistryConsumer tableTypeRegistry,
             final ApplicationRibId applicationRibId, final Ipv4AddressNoZone ipAddress, final RIB rib) {
-        super(rib, applicationRibId.getValue(), APP_PEER_GROUP, PeerRole.Internal,
-                new IpAddressNoZone(ipAddress), Set.of());
+        super(rib, applicationRibId.getValue(), APP_PEER_GROUP, PeerRole.Internal, null, null,
+            new IpAddressNoZone(ipAddress), rib.getLocalTablesKeys(), Set.of(), Map.of());
         this.tableTypeRegistry = requireNonNull(tableTypeRegistry);
-        final RIB targetRib = requireNonNull(rib);
         peerId = RouterIds.createPeerId(ipAddress);
 
-        final YangInstanceIdentifier peerRib = targetRib.getYangRibId().node(PEER_NID)
-            .node(IdentifierUtils.domPeerId(peerId));
+        final var peerRib = rib.getYangRibId().node(PEER_NID).node(IdentifierUtils.domPeerId(peerId));
         adjRibsInId = peerRib.node(ADJRIBIN_NID).node(TABLES_NID).toOptimized();
         peerRibOutIId = peerRib.node(ADJRIBOUT_NID).node(TABLES_NID).toOptimized();
+
+        createDomChain();
     }
 
     public synchronized void instantiateServiceInstance(final DataTreeChangeExtension dataTreeChangeService,
@@ -163,6 +158,11 @@ public class ApplicationPeer extends AbstractPeer implements DOMDataTreeChangeLi
         trackerRegistration = rib.getPeerTracker().registerPeer(this);
     }
 
+    @Override
+    public List<RouteTarget> getMemberships() {
+        return List.of();
+    }
+
     @Override
     public void onInitialData() {
         // FIXME: we really want to (under a synchronized block) to ensure adj-rib-in is completely empty here.

diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPPeer.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPPeer.java
index 0a8aeca1c7ac21914d33613e843aa199353bec67..d62e6498cc8dfb3a9b09d2034a13f4c74c6b4544 100644 (file)
--- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPPeer.java
+++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPPeer.java
@@ -167,12 +167,14 @@ public class BGPPeer extends AbstractPeer implements BGPSessionListener {
             final Set<TablesKey> afiSafisGracefulAdvertized,
             final Map<TablesKey, Integer> llGracefulTablesAdvertised,
             final BgpPeer bgpPeer) {
-        super(rib, Ipv4Util.toStringIP(neighborAddress), peerGroupName, role, clusterId,
-                localAs, neighborAddress, afiSafisAdvertized, afiSafisGracefulAdvertized, llGracefulTablesAdvertised);
+        super(rib, Ipv4Util.toStringIP(neighborAddress), peerGroupName, role, clusterId, localAs, neighborAddress,
+            afiSafisAdvertized, afiSafisGracefulAdvertized, llGracefulTablesAdvertised);
         this.tableTypeRegistry = requireNonNull(tableTypeRegistry);
         this.rib = requireNonNull(rib);
         this.rpcRegistry = rpcRegistry;
         this.bgpPeer = bgpPeer;
+
+        createDomChain();
     }
 
     private static Attributes nextHopToAttribute(final Attributes attrs, final MpReachNlri mpReach) {