From 22895faa7e282c28a06ef4f1b95d431697774958 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Thu, 28 Mar 2024 11:06:59 +0100 Subject: [PATCH] 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 --- .../protocol/bgp/rib/impl/AbstractPeer.java | 18 +--------- .../bgp/rib/impl/ApplicationPeer.java | 36 +++++++++---------- .../protocol/bgp/rib/impl/BGPPeer.java | 6 ++-- 3 files changed, 23 insertions(+), 37 deletions(-) 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 96ef601961..02b9bbfeaf 100644 --- 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 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 afiSafisGracefulAdvertized) { - this(rib, peerName, groupId, role, null, null, neighborAddress, - rib.getLocalTablesKeys(), afiSafisGracefulAdvertized, Collections.emptyMap()); } final synchronized FluentFuture 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 f55b10a5dd..1c530731be 100644 --- 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 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 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 0a8aeca1c7..d62e6498cc 100644 --- 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 afiSafisGracefulAdvertized, final Map 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) { -- 2.36.6