From b0972418d61485a8231f4bea0a88af544dd46084 Mon Sep 17 00:00:00 2001 From: EmmettCox Date: Thu, 8 Aug 2019 12:01:42 +0100 Subject: [PATCH 1/1] Make sure registrations are closed This ensures registrations are closed as soon as they are not needed. JIRA: CONTROLLER-1906 Change-Id: I3a391f202963852f47486b78748c8e2d7e97162a Signed-off-by: EmmettCox Signed-off-by: Robert Varga --- .../controller/remote/rpc/OpsRegistrar.java | 40 ++++++++++++++----- .../remote/rpc/OpsRegistrarTest.java | 4 +- 2 files changed, 33 insertions(+), 11 deletions(-) diff --git a/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/OpsRegistrar.java b/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/OpsRegistrar.java index fdda197c3b..a16bbd4eb5 100644 --- a/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/OpsRegistrar.java +++ b/opendaylight/md-sal/sal-remoterpc-connector/src/main/java/org/opendaylight/controller/remote/rpc/OpsRegistrar.java @@ -11,12 +11,13 @@ import static java.util.Objects.requireNonNull; import akka.actor.Address; import akka.actor.Props; +import java.util.ArrayList; +import java.util.Collection; import java.util.HashMap; import java.util.Map; import java.util.Map.Entry; import java.util.Optional; import org.opendaylight.controller.cluster.common.actor.AbstractUntypedActor; -import org.opendaylight.controller.remote.rpc.registry.ActionRegistry; import org.opendaylight.controller.remote.rpc.registry.ActionRegistry.Messages.UpdateRemoteActionEndpoints; import org.opendaylight.controller.remote.rpc.registry.ActionRegistry.RemoteActionEndpoint; import org.opendaylight.controller.remote.rpc.registry.RpcRegistry.Messages.UpdateRemoteEndpoints; @@ -54,7 +55,9 @@ final class OpsRegistrar extends AbstractUntypedActor { @Override public void postStop() throws Exception { + rpcRegs.values().forEach(ObjectRegistration::close); rpcRegs.clear(); + actionRegs.values().forEach(ObjectRegistration::close); actionRegs.clear(); super.postStop(); @@ -82,26 +85,34 @@ final class OpsRegistrar extends AbstractUntypedActor { * Note that when an RPC moves from one remote node to another, we also do not want to expose the gap, * hence we register all new implementations before closing all registrations. */ + final Collection> prevRegs = new ArrayList<>(rpcEndpoints.size()); + for (Entry> e : rpcEndpoints.entrySet()) { LOG.debug("Updating RPC registrations for {}", e.getKey()); + final ObjectRegistration prevReg; final Optional maybeEndpoint = e.getValue(); if (maybeEndpoint.isPresent()) { final RemoteRpcEndpoint endpoint = maybeEndpoint.get(); final RemoteRpcImplementation impl = new RemoteRpcImplementation(endpoint.getRouter(), config); - rpcRegs.put(e.getKey(), rpcProviderService.registerRpcImplementation(impl, - endpoint.getRpcs())); + prevReg = rpcRegs.put(e.getKey(), rpcProviderService.registerRpcImplementation(impl, + endpoint.getRpcs())); } else { - rpcRegs.remove(e.getKey()); + prevReg = rpcRegs.remove(e.getKey()); + } + + if (prevReg != null) { + prevRegs.add(prevReg); } } + + prevRegs.forEach(ObjectRegistration::close); } /** * Updates the action endpoints, Adding new registrations first before removing previous registrations. */ - private void updateRemoteActionEndpoints(final Map> actionEndpoints) { + private void updateRemoteActionEndpoints(final Map> actionEndpoints) { /* * Updating Action providers is a two-step process. We first add the newly-discovered RPCs and then close * the old registration. This minimizes churn observed by listeners, as they will not observe RPC @@ -110,18 +121,27 @@ final class OpsRegistrar extends AbstractUntypedActor { * Note that when an Action moves from one remote node to another, we also do not want to expose the gap, * hence we register all new implementations before closing all registrations. */ + final Collection> prevRegs = new ArrayList<>(actionEndpoints.size()); + for (Entry> e : actionEndpoints.entrySet()) { - LOG.debug("Updating Action registrations for {}", e.getKey()); + LOG.debug("Updating action registrations for {}", e.getKey()); + final ObjectRegistration prevReg; final Optional maybeEndpoint = e.getValue(); if (maybeEndpoint.isPresent()) { final RemoteActionEndpoint endpoint = maybeEndpoint.get(); final RemoteActionImplementation impl = new RemoteActionImplementation(endpoint.getRouter(), config); - actionRegs.put(e.getKey(), - actionProviderService.registerActionImplementation(impl, endpoint.getActions())); + prevReg = actionRegs.put(e.getKey(), actionProviderService.registerActionImplementation(impl, + endpoint.getActions())); } else { - actionRegs.remove(e.getKey()); + prevReg = actionRegs.remove(e.getKey()); + } + + if (prevReg != null) { + prevRegs.add(prevReg); } } + + prevRegs.forEach(ObjectRegistration::close); } } diff --git a/opendaylight/md-sal/sal-remoterpc-connector/src/test/java/org/opendaylight/controller/remote/rpc/OpsRegistrarTest.java b/opendaylight/md-sal/sal-remoterpc-connector/src/test/java/org/opendaylight/controller/remote/rpc/OpsRegistrarTest.java index 59db730d01..1bbb22ed7a 100644 --- a/opendaylight/md-sal/sal-remoterpc-connector/src/test/java/org/opendaylight/controller/remote/rpc/OpsRegistrarTest.java +++ b/opendaylight/md-sal/sal-remoterpc-connector/src/test/java/org/opendaylight/controller/remote/rpc/OpsRegistrarTest.java @@ -154,6 +154,8 @@ public class OpsRegistrarTest { inOrder.verify(rpcService).registerRpcImplementation(any(RemoteRpcImplementation.class), eq(secondEndpoint.getRpcs())); + // verify first registration is closed + inOrder.verify(oldReg).close(); verifyNoMoreInteractions(rpcService, oldReg, newReg); } @@ -175,7 +177,7 @@ public class OpsRegistrarTest { eq(secondActionEndpoint.getActions())); // verify first registration is closed -// inOrder.verify(oldReg).close(); + inOrder.verify(oldActionReg).close(); verifyNoMoreInteractions(actionService, oldActionReg, newActionReg); } -- 2.36.6