From ca0f3b9f0277d7ae5741aed718a3f8677afae3db Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Sat, 9 Dec 2023 12:25:50 +0100 Subject: [PATCH] Use ServiceGroupIdentifier in ServiceGroup Squashing ServiceGroupIdentifier into a String is a bad idea when interacting with ServiceGroup -- at the end of the day it is its identifier. Switch to using this identifier, but keep maps based on String. Change-Id: I5270afa40f08939e9f9f1e191a036a50aa465678 Signed-off-by: Robert Varga --- .../dom/impl/ActiveServiceGroup.java | 15 ++++---- .../EOSClusterSingletonServiceProvider.java | 35 ++++++++++--------- .../dom/impl/PlaceholderServiceGroup.java | 3 +- .../singleton/dom/impl/ServiceGroup.java | 4 ++- ...OSClusterSingletonServiceProviderTest.java | 12 +++---- .../dom/impl/ActiveServiceGroupTest.java | 17 +++------ 6 files changed, 40 insertions(+), 46 deletions(-) diff --git a/singleton-service/mdsal-singleton-dom-impl/src/main/java/org/opendaylight/mdsal/singleton/dom/impl/ActiveServiceGroup.java b/singleton-service/mdsal-singleton-dom-impl/src/main/java/org/opendaylight/mdsal/singleton/dom/impl/ActiveServiceGroup.java index 6e75cecf92..33179922cc 100644 --- a/singleton-service/mdsal-singleton-dom-impl/src/main/java/org/opendaylight/mdsal/singleton/dom/impl/ActiveServiceGroup.java +++ b/singleton-service/mdsal-singleton-dom-impl/src/main/java/org/opendaylight/mdsal/singleton/dom/impl/ActiveServiceGroup.java @@ -7,7 +7,6 @@ */ package org.opendaylight.mdsal.singleton.dom.impl; -import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkState; import static com.google.common.base.Verify.verify; import static com.google.common.base.Verify.verifyNotNull; @@ -37,6 +36,7 @@ import org.opendaylight.mdsal.eos.common.api.EntityOwnershipStateChange; import org.opendaylight.mdsal.eos.dom.api.DOMEntity; import org.opendaylight.mdsal.eos.dom.api.DOMEntityOwnershipService; import org.opendaylight.mdsal.singleton.common.api.ClusterSingletonService; +import org.opendaylight.mdsal.singleton.common.api.ServiceGroupIdentifier; import org.opendaylight.yangtools.concepts.Registration; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -101,7 +101,7 @@ final class ActiveServiceGroup extends ServiceGroup { private static final Logger LOG = LoggerFactory.getLogger(ActiveServiceGroup.class); private final @NonNull DOMEntityOwnershipService entityOwnershipService; - private final @NonNull String identifier; + private final @NonNull ServiceGroupIdentifier identifier; /* Entity instances */ private final @NonNull DOMEntity serviceEntity; @@ -181,10 +181,9 @@ final class ActiveServiceGroup extends ServiceGroup { * @param parent parent service * @param services Services list */ - ActiveServiceGroup(final String identifier, final DOMEntityOwnershipService entityOwnershipService, + ActiveServiceGroup(final ServiceGroupIdentifier identifier, final DOMEntityOwnershipService entityOwnershipService, final DOMEntity serviceEntity, final DOMEntity cleanupEntity, final List services) { - checkArgument(!identifier.isEmpty(), "Identifier may not be empty"); - this.identifier = identifier; + this.identifier = requireNonNull(identifier); this.entityOwnershipService = requireNonNull(entityOwnershipService); this.serviceEntity = requireNonNull(serviceEntity); this.cleanupEntity = requireNonNull(cleanupEntity); @@ -194,13 +193,13 @@ final class ActiveServiceGroup extends ServiceGroup { } @VisibleForTesting - ActiveServiceGroup(final String identifier, final DOMEntity serviceEntity, + ActiveServiceGroup(final ServiceGroupIdentifier identifier, final DOMEntity serviceEntity, final DOMEntity cleanupEntity, final DOMEntityOwnershipService entityOwnershipService) { this(identifier, entityOwnershipService, serviceEntity, cleanupEntity, ImmutableList.of()); } @Override - public String getIdentifier() { + public ServiceGroupIdentifier getIdentifier() { return identifier; } @@ -288,7 +287,7 @@ final class ActiveServiceGroup extends ServiceGroup { private ClusterSingletonService verifyRegistration(final ServiceRegistration reg) { final var service = reg.getInstance(); - verify(identifier.equals(service.getIdentifier().value())); + verify(identifier.equals(service.getIdentifier())); return service; } diff --git a/singleton-service/mdsal-singleton-dom-impl/src/main/java/org/opendaylight/mdsal/singleton/dom/impl/EOSClusterSingletonServiceProvider.java b/singleton-service/mdsal-singleton-dom-impl/src/main/java/org/opendaylight/mdsal/singleton/dom/impl/EOSClusterSingletonServiceProvider.java index 98d7e70ab7..0e4464cea4 100644 --- a/singleton-service/mdsal-singleton-dom-impl/src/main/java/org/opendaylight/mdsal/singleton/dom/impl/EOSClusterSingletonServiceProvider.java +++ b/singleton-service/mdsal-singleton-dom-impl/src/main/java/org/opendaylight/mdsal/singleton/dom/impl/EOSClusterSingletonServiceProvider.java @@ -31,6 +31,7 @@ import org.opendaylight.mdsal.eos.dom.api.DOMEntityOwnershipListener; import org.opendaylight.mdsal.eos.dom.api.DOMEntityOwnershipService; import org.opendaylight.mdsal.singleton.common.api.ClusterSingletonService; import org.opendaylight.mdsal.singleton.common.api.ClusterSingletonServiceProvider; +import org.opendaylight.mdsal.singleton.common.api.ServiceGroupIdentifier; import org.opendaylight.yangtools.concepts.Registration; import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.NodeIdentifierWithPredicates; import org.osgi.service.component.annotations.Activate; @@ -100,14 +101,15 @@ public final class EOSClusterSingletonServiceProvider @Override public synchronized Registration registerClusterSingletonService(final ClusterSingletonService service) { + final var serviceIdentifier = requireNonNull(service.getIdentifier()); LOG.debug("Call registrationService {} method for ClusterSingletonService Provider {}", service, this); - final var serviceIdentifier = service.getIdentifier().value(); + final var identifierValue = serviceIdentifier.value(); final ServiceGroup serviceGroup; - final var existing = serviceGroupMap.get(serviceIdentifier); + final var existing = serviceGroupMap.get(identifierValue); if (existing == null) { serviceGroup = createGroup(serviceIdentifier, new ArrayList<>(1)); - serviceGroupMap.put(serviceIdentifier, serviceGroup); + serviceGroupMap.put(identifierValue, serviceGroup); try { initializeOrRemoveGroup(serviceGroup); @@ -131,10 +133,11 @@ public final class EOSClusterSingletonServiceProvider return reg; } - private ServiceGroup createGroup(final String serviceIdentifier, final List services) { - return new ActiveServiceGroup(serviceIdentifier, entityOwnershipService, - createEntity(SERVICE_ENTITY_TYPE, serviceIdentifier), - createEntity(CLOSE_SERVICE_ENTITY_TYPE, serviceIdentifier), services); + private ServiceGroup createGroup(final ServiceGroupIdentifier identifier, + final List services) { + return new ActiveServiceGroup(identifier, entityOwnershipService, + createEntity(SERVICE_ENTITY_TYPE, identifier), createEntity(CLOSE_SERVICE_ENTITY_TYPE, identifier), + services); } private void initializeOrRemoveGroup(final ServiceGroup group) throws CandidateAlreadyRegisteredException { @@ -146,11 +149,11 @@ public final class EOSClusterSingletonServiceProvider } } - private void removeRegistration(final String serviceIdentifier, final ServiceRegistration reg) { + private void removeRegistration(final ServiceGroupIdentifier serviceIdentifier, final ServiceRegistration reg) { final PlaceholderServiceGroup placeHolder; final ListenableFuture future; synchronized (this) { - final var lookup = verifyNotNull(serviceGroupMap.get(serviceIdentifier)); + final var lookup = verifyNotNull(serviceGroupMap.get(serviceIdentifier.value())); future = lookup.unregisterService(reg); if (future == null) { return; @@ -177,17 +180,17 @@ public final class EOSClusterSingletonServiceProvider final var services = placeHolder.getServices(); if (services.isEmpty()) { // No services, we are all done - if (serviceGroupMap.remove(identifier, placeHolder)) { + if (serviceGroupMap.remove(identifier.value(), placeHolder)) { LOG.debug("Service group {} removed", placeHolder); } else { - LOG.debug("Service group {} superseded by {}", placeHolder, serviceGroupMap.get(identifier)); + LOG.debug("Service group {} superseded by {}", placeHolder, serviceGroupMap.get(identifier.value())); } return; } // Placeholder is being retired, we are reusing its services as the seed for the group. final var group = createGroup(identifier, services); - verify(serviceGroupMap.replace(identifier, placeHolder, group)); + verify(serviceGroupMap.replace(identifier.value(), placeHolder, group)); placeHolder.setSuccessor(group); LOG.debug("Service group upgraded from {} to {}", placeHolder, group); @@ -214,7 +217,7 @@ public final class EOSClusterSingletonServiceProvider } @VisibleForTesting - static String getServiceIdentifierFromEntity(final DOMEntity entity) { + static @NonNull String getServiceIdentifierFromEntity(final DOMEntity entity) { final var yii = entity.getIdentifier(); final var niiwp = (NodeIdentifierWithPredicates) yii.getLastPathArgument(); return niiwp.values().iterator().next().toString(); @@ -224,11 +227,11 @@ public final class EOSClusterSingletonServiceProvider * Creates an extended {@link DOMEntity} instance. * * @param entityType the type of the entity - * @param entityIdentifier the identifier of the entity + * @param sgi the identifier of the entity * @return instance of Entity extended GenericEntity type */ @VisibleForTesting - static DOMEntity createEntity(final String entityType, final String entityIdentifier) { - return new DOMEntity(entityType, entityIdentifier); + static DOMEntity createEntity(final String entityType, final ServiceGroupIdentifier sgi) { + return new DOMEntity(entityType, sgi.value()); } } diff --git a/singleton-service/mdsal-singleton-dom-impl/src/main/java/org/opendaylight/mdsal/singleton/dom/impl/PlaceholderServiceGroup.java b/singleton-service/mdsal-singleton-dom-impl/src/main/java/org/opendaylight/mdsal/singleton/dom/impl/PlaceholderServiceGroup.java index cb3c45732c..8a27e388c2 100644 --- a/singleton-service/mdsal-singleton-dom-impl/src/main/java/org/opendaylight/mdsal/singleton/dom/impl/PlaceholderServiceGroup.java +++ b/singleton-service/mdsal-singleton-dom-impl/src/main/java/org/opendaylight/mdsal/singleton/dom/impl/PlaceholderServiceGroup.java @@ -18,6 +18,7 @@ import java.util.List; import org.opendaylight.mdsal.eos.common.api.CandidateAlreadyRegisteredException; import org.opendaylight.mdsal.eos.common.api.EntityOwnershipStateChange; import org.opendaylight.mdsal.eos.dom.api.DOMEntity; +import org.opendaylight.mdsal.singleton.common.api.ServiceGroupIdentifier; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -40,7 +41,7 @@ final class PlaceholderServiceGroup extends ServiceGroup { } @Override - public String getIdentifier() { + public ServiceGroupIdentifier getIdentifier() { return previous.getIdentifier(); } diff --git a/singleton-service/mdsal-singleton-dom-impl/src/main/java/org/opendaylight/mdsal/singleton/dom/impl/ServiceGroup.java b/singleton-service/mdsal-singleton-dom-impl/src/main/java/org/opendaylight/mdsal/singleton/dom/impl/ServiceGroup.java index bf8f5d3577..ce19b2f403 100644 --- a/singleton-service/mdsal-singleton-dom-impl/src/main/java/org/opendaylight/mdsal/singleton/dom/impl/ServiceGroup.java +++ b/singleton-service/mdsal-singleton-dom-impl/src/main/java/org/opendaylight/mdsal/singleton/dom/impl/ServiceGroup.java @@ -13,6 +13,7 @@ import org.opendaylight.mdsal.eos.common.api.CandidateAlreadyRegisteredException import org.opendaylight.mdsal.eos.common.api.EntityOwnershipStateChange; import org.opendaylight.mdsal.eos.dom.api.DOMEntity; import org.opendaylight.mdsal.singleton.common.api.ClusterSingletonService; +import org.opendaylight.mdsal.singleton.common.api.ServiceGroupIdentifier; import org.opendaylight.yangtools.concepts.Identifiable; /** @@ -24,7 +25,8 @@ import org.opendaylight.yangtools.concepts.Identifiable; * ensure a service group instance has fully closed prior to relinquishing service ownership. To achieve ownership * of the service group, a service group candidate must hold ownership of both these entities. */ -abstract sealed class ServiceGroup implements Identifiable permits ActiveServiceGroup, PlaceholderServiceGroup { +abstract sealed class ServiceGroup implements Identifiable + permits ActiveServiceGroup, PlaceholderServiceGroup { /** * This method must be called once on startup to initialize this group and register the relevant group entity * candidate. It means create relevant Group Entity Candidate Registration. diff --git a/singleton-service/mdsal-singleton-dom-impl/src/test/java/org/opendaylight/mdsal/singleton/dom/impl/AbstractEOSClusterSingletonServiceProviderTest.java b/singleton-service/mdsal-singleton-dom-impl/src/test/java/org/opendaylight/mdsal/singleton/dom/impl/AbstractEOSClusterSingletonServiceProviderTest.java index 8f4c22fff0..bf49570e82 100644 --- a/singleton-service/mdsal-singleton-dom-impl/src/test/java/org/opendaylight/mdsal/singleton/dom/impl/AbstractEOSClusterSingletonServiceProviderTest.java +++ b/singleton-service/mdsal-singleton-dom-impl/src/test/java/org/opendaylight/mdsal/singleton/dom/impl/AbstractEOSClusterSingletonServiceProviderTest.java @@ -60,8 +60,6 @@ abstract class AbstractEOSClusterSingletonServiceProviderTest { } static class TestClusterSingletonService implements ClusterSingletonService { - private static final @NonNull ServiceGroupIdentifier SERVICE_ID = new ServiceGroupIdentifier(SERVICE_NAME); - private TestClusterSingletonServiceState serviceState = TestClusterSingletonServiceState.INITIALIZED; @Override @@ -85,7 +83,8 @@ abstract class AbstractEOSClusterSingletonServiceProviderTest { } } - static final @NonNull String SERVICE_NAME = "testServiceName"; + private static final @NonNull String SERVICE_NAME = "testServiceName"; + private static final @NonNull ServiceGroupIdentifier SERVICE_ID = new ServiceGroupIdentifier(SERVICE_NAME); static final @NonNull DOMEntity ENTITY = new DOMEntity(SERVICE_ENTITY_TYPE, SERVICE_NAME); static final @NonNull DOMEntity DOUBLE_ENTITY = new DOMEntity(CLOSE_SERVICE_ENTITY_TYPE, SERVICE_NAME); @@ -158,10 +157,9 @@ abstract class AbstractEOSClusterSingletonServiceProviderTest { */ @Test public void makeEntityClusterSingletonServiceProviderTest() { - final var testEntity = EOSClusterSingletonServiceProvider.createEntity(SERVICE_ENTITY_TYPE, SERVICE_NAME); - assertEquals(ENTITY, testEntity); - final var testDbEn = EOSClusterSingletonServiceProvider.createEntity(CLOSE_SERVICE_ENTITY_TYPE, SERVICE_NAME); - assertEquals(DOUBLE_ENTITY, testDbEn); + assertEquals(ENTITY, EOSClusterSingletonServiceProvider.createEntity(SERVICE_ENTITY_TYPE, SERVICE_ID)); + assertEquals(DOUBLE_ENTITY, + EOSClusterSingletonServiceProvider.createEntity(CLOSE_SERVICE_ENTITY_TYPE, SERVICE_ID)); } /** diff --git a/singleton-service/mdsal-singleton-dom-impl/src/test/java/org/opendaylight/mdsal/singleton/dom/impl/ActiveServiceGroupTest.java b/singleton-service/mdsal-singleton-dom-impl/src/test/java/org/opendaylight/mdsal/singleton/dom/impl/ActiveServiceGroupTest.java index f9a919fdae..88e3f4e366 100644 --- a/singleton-service/mdsal-singleton-dom-impl/src/test/java/org/opendaylight/mdsal/singleton/dom/impl/ActiveServiceGroupTest.java +++ b/singleton-service/mdsal-singleton-dom-impl/src/test/java/org/opendaylight/mdsal/singleton/dom/impl/ActiveServiceGroupTest.java @@ -99,7 +99,7 @@ public class ActiveServiceGroupTest { } }; - singletonServiceGroup = new ActiveServiceGroup(SERVICE_IDENTIFIER, MAIN_ENTITY, CLOSE_ENTITY, mockEosService); + singletonServiceGroup = new ActiveServiceGroup(SERVICE_GROUP_IDENT, MAIN_ENTITY, CLOSE_ENTITY, mockEosService); } /** @@ -111,22 +111,13 @@ public class ActiveServiceGroupTest { () -> new ActiveServiceGroup(null, MAIN_ENTITY, CLOSE_ENTITY, mockEosService)); } - /** - * Test empty ServiceIdent input for new ServiceGroup instance. - */ - @Test - public void instantiationClusterSingletonServiceGroupEmptyIdentTest() { - assertThrows(IllegalArgumentException.class, - () -> new ActiveServiceGroup("", MAIN_ENTITY, CLOSE_ENTITY, mockEosService)); - } - /** * Test NULL MainEntity input for new ServiceGroup instance. */ @Test public void instantiationClusterSingletonServiceGroupNullMainEntityTest() { assertThrows(NullPointerException.class, - () -> new ActiveServiceGroup(SERVICE_IDENTIFIER, null, CLOSE_ENTITY, mockEosService)); + () -> new ActiveServiceGroup(SERVICE_GROUP_IDENT, null, CLOSE_ENTITY, mockEosService)); } /** @@ -135,7 +126,7 @@ public class ActiveServiceGroupTest { @Test public void instantiationClusterSingletonServiceGroupNullCloseEntityTest() { assertThrows(NullPointerException.class, - () -> new ActiveServiceGroup(SERVICE_IDENTIFIER, MAIN_ENTITY, null, mockEosService)); + () -> new ActiveServiceGroup(SERVICE_GROUP_IDENT, MAIN_ENTITY, null, mockEosService)); } /** @@ -144,7 +135,7 @@ public class ActiveServiceGroupTest { @Test public void instantiationClusterSingletonServiceGroupNullEOS_Test() { assertThrows(NullPointerException.class, - () -> new ActiveServiceGroup(SERVICE_IDENTIFIER, MAIN_ENTITY, CLOSE_ENTITY, null)); + () -> new ActiveServiceGroup(SERVICE_GROUP_IDENT, MAIN_ENTITY, CLOSE_ENTITY, null)); } /** -- 2.36.6