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 <robert.varga@pantheon.tech>

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 6e75cecf923ed4da5b83e72283f2e94b7ec5faec..33179922cc8fc3b2657e9d0dcfb8f38965c8a84b 100644 (file)
--- 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<ServiceRegistration> 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 98d7e70ab732b25e458f3e3a239324754dff71ad..0e4464cea4e894275632e7179a1b47075283e324 100644 (file)
--- 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<ServiceRegistration> 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<ServiceRegistration> 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 cb3c45732c4a195639fd6ca2563332ce115e5dd0..8a27e388c2c0a05f261027b328bcc9e9d07f2eb3 100644 (file)
--- 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 bf8f5d3577654764f279da01bcbb102fe9c99e50..ce19b2f40324285b0d6fdda3ad69cf05e5964052 100644 (file)
--- 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<String> permits ActiveServiceGroup, PlaceholderServiceGroup {
+abstract sealed class ServiceGroup implements Identifiable<ServiceGroupIdentifier>
+        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 8f4c22fff0e60073b445d9ddd2418686999de652..bf49570e8229d2ebfea80e9141d16681434c5972 100644 (file)
--- 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 f9a919fdae93806546cceff75f0c7db8af46a7bb..88e3f4e3661ff57f376d37c04262cdc44241d886 100644 (file)
--- 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));
     }
 
     /**