From: Robert Varga Date: Tue, 2 Aug 2022 21:29:32 +0000 (+0200) Subject: Integrate TopologyStatsProviderImpl X-Git-Tag: v0.18.0~7 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?a=commitdiff_plain;ds=sidebyside;h=dbec59013da2778137dc9b5de005e93c48ec977a;p=bgpcep.git Integrate TopologyStatsProviderImpl Provider is an implementation detail of PCEPTopologyTracker, make sure it is managed internally. Also rename the interface and the class a bit and clean up the shutdown semantics. JIRA: BGPCEP-1011 Change-Id: I9c0c21c4ba47b8e6fec05765027462adbb8ee393 Signed-off-by: Robert Varga --- diff --git a/pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/AbstractTopologySessionListener.java b/pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/AbstractTopologySessionListener.java index ded520e4b7..cf90355f2a 100644 --- a/pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/AbstractTopologySessionListener.java +++ b/pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/AbstractTopologySessionListener.java @@ -100,8 +100,8 @@ public abstract class AbstractTopologySessionListener implements TopologySession private final Map requests = new HashMap<>(); @GuardedBy("this") private final Map lspData = new ConcurrentHashMap<>(); - private final TopologySessionStatsRegistry statsProvider; private final ServerSessionManager serverSessionManager; + private final SessionStateRegistry stateRegistry; private InstanceIdentifier pccIdentifier; @GuardedBy("this") @@ -114,9 +114,9 @@ public abstract class AbstractTopologySessionListener implements TopologySession @GuardedBy("this") private boolean triggeredResyncInProcess; - AbstractTopologySessionListener(final TopologySessionStatsRegistry statsProvider, + AbstractTopologySessionListener(final SessionStateRegistry stateRegistry, final ServerSessionManager serverSessionManager) { - this.statsProvider = requireNonNull(statsProvider); + this.stateRegistry = requireNonNull(stateRegistry); this.serverSessionManager = requireNonNull(serverSessionManager); } @@ -181,7 +181,7 @@ public abstract class AbstractTopologySessionListener implements TopologySession state.storeNode(topologyAugment, new Node1Builder().setPathComputationClient(pccBuilder.build()).build(), psession); - listenerState = statsProvider.bind(nodeId, new SessionStateImpl(this, psession)); + listenerState = stateRegistry.bind(nodeId, new SessionStateImpl(this, psession)); LOG.info("Session with {} attached to topology node {}", peerAddress, nodeId); } } diff --git a/pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/PCEPTopologyProviderDependencies.java b/pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/PCEPTopologyProviderDependencies.java index 2ff1f46906..d3c360dd0b 100644 --- a/pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/PCEPTopologyProviderDependencies.java +++ b/pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/PCEPTopologyProviderDependencies.java @@ -45,7 +45,7 @@ interface PCEPTopologyProviderDependencies { * * @return TopologySessionStateRegistry */ - TopologySessionStatsRegistry getStateRegistry(); + SessionStateRegistry getStateRegistry(); /** * PCE Server Provider. diff --git a/pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/PCEPTopologySessionListener.java b/pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/PCEPTopologySessionListener.java index e2965ccd74..117799501a 100644 --- a/pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/PCEPTopologySessionListener.java +++ b/pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/PCEPTopologySessionListener.java @@ -134,9 +134,9 @@ class PCEPTopologySessionListener extends AbstractTopologySessionListener { /** * Creates a new stateful topology session listener for given server session manager. */ - PCEPTopologySessionListener(final TopologySessionStatsRegistry statsProvider, + PCEPTopologySessionListener(final SessionStateRegistry stateRegistry, final ServerSessionManager serverSessionManager, final PceServerProvider pceServerProvider) { - super(statsProvider, serverSessionManager); + super(stateRegistry, serverSessionManager); // FIXME: requireNonNull(), except tests need to be updated this.pceServerProvider = pceServerProvider; } diff --git a/pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/PCEPTopologyTracker.java b/pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/PCEPTopologyTracker.java index e5a8d4945a..5c65d5fd93 100644 --- a/pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/PCEPTopologyTracker.java +++ b/pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/PCEPTopologyTracker.java @@ -19,6 +19,7 @@ import java.util.Collection; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import org.checkerframework.checker.lock.qual.GuardedBy; import org.eclipse.jdt.annotation.NonNull; @@ -55,12 +56,14 @@ public final class PCEPTopologyTracker // Services we are using final @NonNull InstructionSchedulerFactory instructionSchedulerFactory; final @NonNull ClusterSingletonServiceProvider singletonService; - private final @NonNull TopologySessionStatsRegistry stateRegistry; private final @NonNull RpcProviderService rpcProviderRegistry; private final @NonNull PceServerProvider pceServerProvider; private final @NonNull PCEPDispatcher pcepDispatcher; private final @NonNull DataBroker dataBroker; + // Statistics provider + private final @NonNull TopologyStatsProvider statsProvider; + // Timer used for RPC timeouts and session statistics scheduling private final @NonNull HashedWheelTimer privateTimer = new HashedWheelTimer(); private final @NonNull Timer timer = new Timer() { @@ -99,15 +102,15 @@ public final class PCEPTopologyTracker public PCEPTopologyTracker(final DataBroker dataBroker, final ClusterSingletonServiceProvider singletonService, final RpcProviderService rpcProviderRegistry, final PCEPDispatcher pcepDispatcher, - final InstructionSchedulerFactory instructionSchedulerFactory, - final TopologySessionStatsRegistry stateRegistry, final PceServerProvider pceServerProvider) { + final InstructionSchedulerFactory instructionSchedulerFactory, final PceServerProvider pceServerProvider, + final int updateIntervalSeconds) { this.dataBroker = requireNonNull(dataBroker); this.singletonService = requireNonNull(singletonService); this.rpcProviderRegistry = requireNonNull(rpcProviderRegistry); this.pcepDispatcher = requireNonNull(pcepDispatcher); this.instructionSchedulerFactory = requireNonNull(instructionSchedulerFactory); - this.stateRegistry = requireNonNull(stateRegistry); this.pceServerProvider = requireNonNull(pceServerProvider); + statsProvider = new TopologyStatsProvider(dataBroker, updateIntervalSeconds); reg = dataBroker.registerDataTreeChangeListener(DataTreeIdentifier.create(LogicalDatastoreType.CONFIGURATION, InstanceIdentifier.builder(NetworkTopology.class).child(Topology.class).child(TopologyTypes.class) @@ -131,8 +134,8 @@ public final class PCEPTopologyTracker } @Override - public TopologySessionStatsRegistry getStateRegistry() { - return stateRegistry; + public SessionStateRegistry getStateRegistry() { + return statsProvider; } @Override @@ -167,6 +170,15 @@ public final class PCEPTopologyTracker LOG.warn("Stopped timer with {} remaining tasks", cancelledTasks); } + try { + statsProvider.shutdown(); + } catch (ExecutionException e) { + LOG.warn("Failed to close statistics provider", e); + } catch (InterruptedException e) { + LOG.warn("Interrupted while waiting for statistics provider shutdown", e); + Thread.currentThread().interrupt(); + } + LOG.info("PCEP Topology tracker shut down"); } diff --git a/pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/TopologySessionStatsRegistry.java b/pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/SessionStateRegistry.java similarity index 97% rename from pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/TopologySessionStatsRegistry.java rename to pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/SessionStateRegistry.java index c6f3114339..aabd0124ef 100644 --- a/pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/TopologySessionStatsRegistry.java +++ b/pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/SessionStateRegistry.java @@ -18,7 +18,7 @@ import org.opendaylight.yangtools.yang.binding.KeyedInstanceIdentifier; /** * Topology Node Sessions stats handler. Will store Session stats on DS per each Topology Node registered. */ -interface TopologySessionStatsRegistry { +interface SessionStateRegistry { /** * Register session to Session stats Registry handler. * diff --git a/pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/TopologyStatsProviderImpl.java b/pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/TopologyStatsProvider.java similarity index 93% rename from pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/TopologyStatsProviderImpl.java rename to pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/TopologyStatsProvider.java index 5e2b5ade2f..d10b9b9a86 100644 --- a/pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/TopologyStatsProviderImpl.java +++ b/pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/TopologyStatsProvider.java @@ -46,9 +46,8 @@ import org.opendaylight.yangtools.yang.binding.KeyedInstanceIdentifier; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -public final class TopologyStatsProviderImpl implements TopologySessionStatsRegistry, TransactionChainListener, - AutoCloseable { - private static final Logger LOG = LoggerFactory.getLogger(TopologyStatsProviderImpl.class); +final class TopologyStatsProvider implements SessionStateRegistry, TransactionChainListener { + private static final Logger LOG = LoggerFactory.getLogger(TopologyStatsProvider.class); // This tracking looks weird. It essentially tracks when there is a pending delete transaction and skips updates -- // which is the okay part. The problem is that if the remove operation fails for some reason, we do not retry @@ -69,11 +68,11 @@ public final class TopologyStatsProviderImpl implements TopologySessionStatsRegi @GuardedBy("this") private final ScheduledFuture scheduleTask; - public TopologyStatsProviderImpl(final DataBroker dataBroker, final int updateIntervalSeconds) { + TopologyStatsProvider(final DataBroker dataBroker, final int updateIntervalSeconds) { this(dataBroker, updateIntervalSeconds, Executors.newScheduledThreadPool(1)); } - public TopologyStatsProviderImpl(final DataBroker dataBroker, final int updateIntervalSeconds, + TopologyStatsProvider(final DataBroker dataBroker, final int updateIntervalSeconds, final ScheduledExecutorService scheduler) { this.dataBroker = requireNonNull(dataBroker); LOG.info("Initializing TopologyStatsProvider service."); @@ -85,17 +84,18 @@ public final class TopologyStatsProviderImpl implements TopologySessionStatsRegi }, 0, updateIntervalSeconds, TimeUnit.SECONDS); } - @Override - public void close() throws InterruptedException, ExecutionException { + // FIXME: there should be no further tasks, hence this should not be needed + // FIXME: if it ends up being needed, it needs to be asynchronous + void shutdown() throws InterruptedException, ExecutionException { if (scheduleTask.cancel(true)) { LOG.info("Closing TopologyStatsProvider service."); - shutdown(); + lockedShutdown(); } else { LOG.debug("TopologyStatsProvider already shut down"); } } - private synchronized void shutdown() throws InterruptedException, ExecutionException { + private synchronized void lockedShutdown() throws InterruptedException, ExecutionException { // Try to get a transaction chain and indicate we are done final TransactionChain chain = accessChain(); transactionChain = null; @@ -250,7 +250,7 @@ public final class TopologyStatsProviderImpl implements TopologySessionStatsRegi @Override protected void removeRegistration() { - TopologyStatsProviderImpl.this.removeRegistration(this); + TopologyStatsProvider.this.removeRegistration(this); } } } diff --git a/pcep/topology/topology-provider/src/main/resources/OSGI-INF/blueprint/pcep-topology.xml b/pcep/topology/topology-provider/src/main/resources/OSGI-INF/blueprint/pcep-topology.xml index aff99bc8c1..87a3493967 100644 --- a/pcep/topology/topology-provider/src/main/resources/OSGI-INF/blueprint/pcep-topology.xml +++ b/pcep/topology/topology-provider/src/main/resources/OSGI-INF/blueprint/pcep-topology.xml @@ -16,6 +16,9 @@ + + @@ -24,8 +27,10 @@ - + + + - - - - - - - - - diff --git a/pcep/topology/topology-provider/src/test/java/org/opendaylight/bgpcep/pcep/topology/provider/AbstractPCEPSessionTest.java b/pcep/topology/topology-provider/src/test/java/org/opendaylight/bgpcep/pcep/topology/provider/AbstractPCEPSessionTest.java index ca56fc3a77..24e39b3d5d 100644 --- a/pcep/topology/topology-provider/src/test/java/org/opendaylight/bgpcep/pcep/topology/provider/AbstractPCEPSessionTest.java +++ b/pcep/topology/topology-provider/src/test/java/org/opendaylight/bgpcep/pcep/topology/provider/AbstractPCEPSessionTest.java @@ -97,7 +97,7 @@ public abstract class AbstractPCEPSessionTest extends AbstractConcurrentDataBrok @Mock private ChannelFuture channelFuture; @Mock - private TopologySessionStatsRegistry statsRegistry; + private SessionStateRegistry stateRegistry; @Mock private PCEPTopologyProviderDependencies topologyDependencies; @Mock @@ -121,7 +121,7 @@ public abstract class AbstractPCEPSessionTest extends AbstractConcurrentDataBrok any(ChannelHandler.class)); doReturn(eventLoop).when(clientListener).eventLoop(); doAnswer(inv -> NoOpObjectRegistration.of(inv.getArgument(1, PcepSessionState.class))) - .when(statsRegistry).bind(any(), any()); + .when(stateRegistry).bind(any(), any()); doReturn(null).when(eventLoop).schedule(any(Runnable.class), any(long.class), any(TimeUnit.class)); doReturn(true).when(clientListener).isActive(); final InetSocketAddress ra = new InetSocketAddress(testAddress, 4189); @@ -132,7 +132,7 @@ public abstract class AbstractPCEPSessionTest extends AbstractConcurrentDataBrok doReturn(mock(ChannelFuture.class)).when(clientListener).close(); doReturn(getDataBroker()).when(topologyDependencies).getDataBroker(); - doReturn(statsRegistry).when(topologyDependencies).getStateRegistry(); + doReturn(stateRegistry).when(topologyDependencies).getStateRegistry(); doReturn(timer).when(topologyDependencies).getTimer(); doReturn(null).when(topologyDependencies).getPceServerProvider();