Bug 5540 - ConvertorManager optimization, concurrency

- Split ConvertorManager functionality to interfaces ConvertorRegistrator and
  ConvertorExecutor
- Changed convertor maps to concurrent hash maps
- registerConvertor now returns old convertor if already registered instead of boolean
- Better type extraction from collections
- Removed duplicate map lookups
- Better log messages

Change-Id: I38c6bc119861fef844535c50740c65fa7ae43192
Signed-off-by: Tomas Slusny <tomas.slusny@pantheon.sk>

diff --git a/openflowplugin/src/main/java/org/opendaylight/openflowplugin/openflow/md/core/sal/convertor/ConvertorExecutor.java b/openflowplugin/src/main/java/org/opendaylight/openflowplugin/openflow/md/core/sal/convertor/ConvertorExecutor.java
new file mode 100644 (file)
index 0000000..191ad4b
--- /dev/null
+++ b/openflowplugin/src/main/java/org/opendaylight/openflowplugin/openflow/md/core/sal/convertor/ConvertorExecutor.java
@@ -0,0 +1,36 @@
+/*
+ * Copyright (c) 2016 Cisco Systems, Inc. and others.  All rights reserved.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v1.0 which accompanies this distribution,
+ * and is available at http://www.eclipse.org/legal/epl-v10.html
+ */
+
+package org.opendaylight.openflowplugin.openflow.md.core.sal.convertor;
+
+import java.util.Optional;
+import org.opendaylight.openflowplugin.openflow.md.core.sal.convertor.common.ConvertorData;
+
+public interface ConvertorExecutor {
+    /**
+     * Lookup and use convertor by specified type, then converts source and returns converted result
+     *
+     * @param <FROM> the source type
+     * @param <TO>   the result type
+     * @param source the source
+     * @return the result (can be empty, if no convertor was found)
+     */
+    <FROM, TO> Optional<TO> convert(final FROM source);
+
+    /**
+     * Lookup and use convertor by specified type, then converts source and returns converted result
+     *
+     * @param <FROM> the source type
+     * @param <TO>   the result type
+     * @param <DATA> the data type
+     * @param source the source
+     * @param data   convertor data
+     * @return the result (can be empty, if no convertor was found)
+     */
+    <FROM, TO, DATA extends ConvertorData> Optional<TO> convert(final FROM source, final DATA data);
+}
\ No newline at end of file

diff --git a/openflowplugin/src/main/java/org/opendaylight/openflowplugin/openflow/md/core/sal/convertor/ConvertorManager.java b/openflowplugin/src/main/java/org/opendaylight/openflowplugin/openflow/md/core/sal/convertor/ConvertorManager.java
index aea5ec7ec8853b12e016c4c5edd26b19c89f93b9..b9981b46fbc3cce7da006f6af354177252ab8556 100644 (file)
--- a/openflowplugin/src/main/java/org/opendaylight/openflowplugin/openflow/md/core/sal/convertor/ConvertorManager.java
+++ b/openflowplugin/src/main/java/org/opendaylight/openflowplugin/openflow/md/core/sal/convertor/ConvertorManager.java
@@ -10,12 +10,11 @@ package org.opendaylight.openflowplugin.openflow.md.core.sal.convertor;
 
 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.HashMap;
-import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.concurrent.ConcurrentHashMap;
 import org.opendaylight.openflowplugin.openflow.md.core.sal.convertor.action.ActionConvertor;
 import org.opendaylight.openflowplugin.openflow.md.core.sal.convertor.action.ActionResponseConvertor;
 import org.opendaylight.openflowplugin.openflow.md.core.sal.convertor.common.Convertor;
@@ -32,7 +31,7 @@ import org.slf4j.LoggerFactory;
 /**
  * Manages various convertors and allows to use them all in one generic way
  */
-public class ConvertorManager {
+public class ConvertorManager implements ConvertorRegistrator, ConvertorExecutor {
     private static final Logger LOG = LoggerFactory.getLogger(ConvertorManager.class);
     private static ConvertorManager INSTANCE;
 
@@ -65,8 +64,8 @@ public class ConvertorManager {
 
     // Cache, that holds all registered convertors, but they can have multiple keys,
     // based on instanceof checks in the convert method
-    private Map<Class<?>, Convertor> convertors = new HashMap<>();
-    private Map<Class<?>, ParametrizedConvertor> parametrizedConvertors = new HashMap<>();
+    private Map<Class<?>, Convertor> convertors = new ConcurrentHashMap<>();
+    private Map<Class<?>, ParametrizedConvertor> parametrizedConvertors = new ConcurrentHashMap<>();
 
     private ConvertorManager() {
         // Hiding implicit constructor
@@ -81,169 +80,121 @@ public class ConvertorManager {
         return INSTANCE;
     }
 
-    /**
-     * Register convertor.
-     *
-     * @param convertor the convertor
-     * @return if registration was successful
-     */
-    public boolean registerConvertor(final Convertor convertor) {
+    @Override
+    public Convertor registerConvertor(final Convertor convertor) {
         final Class<?> type = convertor.getType();
+        final Convertor result = convertors.get(type);
 
-        if (convertors.containsKey(type)) {
+        if (Objects.isNull(result)) {
+            convertorKeys.add(type);
+            convertors.put(type, convertor);
+            LOG.debug("{} is now converted by {}", type, convertor);
+        } else {
             LOG.warn("Convertor for type {} is already registered", type);
-            return false;
         }
 
-        convertorKeys.add(type);
-        convertors.put(type, convertor);
-        LOG.debug("{} is now converted by {}", type, convertor);
-        return true;
+        return result;
     }
 
-    /**
-     * Register convertor.
-     *
-     * @param convertor the convertor
-     * @return if registration was successful
-     */
-    public boolean registerConvertor(final ParametrizedConvertor convertor) {
+    @Override
+    public ParametrizedConvertor registerConvertor(final ParametrizedConvertor convertor) {
         final Class<?> type = convertor.getType();
+        final ParametrizedConvertor result = parametrizedConvertors.get(type);
 
-        if (parametrizedConvertors.containsKey(type)) {
+        if (Objects.isNull(result)) {
+            parametrizedConvertorKeys.add(type);
+            parametrizedConvertors.put(type, convertor);
+            LOG.debug("{} is now converted by {}", type, convertor);
+        } else {
             LOG.warn("Convertor for type {} is already registered", type);
-            return false;
         }
 
-        parametrizedConvertorKeys.add(type);
-        parametrizedConvertors.put(type, convertor);
-        LOG.debug("{} is now converted by {}", type, convertor);
-        return true;
+        return result;
     }
 
-    /**
-     * Lookup and use convertor by specified type, then converts source and returns converted result
-     *
-     * @param <FROM> the source type
-     * @param <TO>   the result type
-     * @param source the source
-     * @return the result (can be empty, if no convertor was found)
-     */
+    @Override
     @SuppressWarnings("unchecked")
     public <FROM, TO> Optional<TO> convert(final FROM source) {
-        if (Objects.isNull(source)) {
-            LOG.trace("Cannot convert source, because it is null");
-            return Optional.empty();
-        }
-
-        Class<?> type = source.getClass();
-
-        if (source instanceof Collection) {
-            final Iterator it = ((Collection) source).iterator();
-            Object next = null;
-
-            if (it.hasNext()) {
-                next = it.next();
-            }
-
-            if (Objects.isNull(next)) {
-                LOG.trace("Cannot convert {}, because it is empty collection", type);
-                return Optional.empty();
-            }
+        final Optional<Class<?>> optionalType = extractType(source);
 
-            type = next.getClass();
+        if (!optionalType.isPresent()) {
+            LOG.trace("Cannot convert {}", source);
+            return Optional.empty();
         }
 
-        Convertor convertor = null;
-
-        if (!convertors.containsKey(type)) {
-            boolean found = false;
+        final Class<?> type = optionalType.get();
+        Convertor convertor = convertors.get(type);
 
-            for (Class<?> key : convertorKeys) {
+        if (Objects.isNull(convertor)) {
+            for (final Class<?> key : convertorKeys) {
                 if (key.isAssignableFrom(type)) {
                     convertor = convertors.get(key);
                     convertors.put(type, convertor);
                     LOG.debug("{} is now converted by {}", type, convertor);
-                    found = true;
                     break;
                 }
             }
 
-            if (!found) {
-                LOG.error("Convertor for {} not found", type);
+            if (Objects.isNull(convertor)) {
+                LOG.warn("Convertor for {} not found", type);
                 return Optional.empty();
             }
         }
 
-        if (Objects.isNull(convertor)) {
-            convertor = convertors.get(type);
-        }
-
-        final Object result = convertor.convert(source);
-        return Optional.of((TO) result);
+        return Optional.of((TO) convertor.convert(source));
     }
 
-    /**
-     * Lookup and use convertor by specified type, then converts source and returns converted result
-     *
-     * @param <FROM> the source type
-     * @param <TO>   the result type
-     * @param <DATA> the data type
-     * @param source the source
-     * @param data   convertor data
-     * @return the result (can be empty, if no convertor was found)
-     */
+    @Override
     @SuppressWarnings("unchecked")
     public <FROM, TO, DATA extends ConvertorData> Optional<TO> convert(final FROM source, final DATA data) {
-        if (Objects.isNull(source)) {
-            LOG.trace("Cannot convert source, because it is null");
-            return Optional.empty();
-        }
-
-        Class<?> type = source.getClass();
-
-        if (source instanceof Collection) {
-            final Iterator it = ((Collection) source).iterator();
-            Object next = null;
+        final Optional<Class<?>> optionalType = extractType(source);
 
-            if (it.hasNext()) {
-                next = it.next();
-            }
-
-            if (Objects.isNull(next)) {
-                LOG.trace("Cannot convert {}, because it is empty collection", type);
-                return Optional.empty();
-            }
-
-            type = next.getClass();
+        if (!optionalType.isPresent()) {
+            LOG.trace("Cannot convert {}", source);
+            return Optional.empty();
         }
 
-        ParametrizedConvertor convertor = null;
-
-        if (!parametrizedConvertors.containsKey(type)) {
-            boolean found = false;
+        final Class<?> type = optionalType.get();
+        ParametrizedConvertor convertor = parametrizedConvertors.get(type);
 
-            for (Class<?> key : parametrizedConvertorKeys) {
+        if (Objects.isNull(convertor)) {
+            for (final Class<?> key : parametrizedConvertorKeys) {
                 if (key.isAssignableFrom(type)) {
                     convertor = parametrizedConvertors.get(key);
                     parametrizedConvertors.put(type, convertor);
                     LOG.debug("{} is now converted by {}", type, convertor);
-                    found = true;
                     break;
                 }
             }
 
-            if (!found) {
-                LOG.error("Convertor for {} not found", type);
+            if (Objects.isNull(convertor)) {
+                LOG.warn("Convertor for {} not found", type);
                 return Optional.empty();
             }
         }
 
-        if (Objects.isNull(convertor)) {
-            convertor = parametrizedConvertors.get(type);
+        return Optional.of((TO) convertor.convert(source, data));
+    }
+
+    private <FROM> Optional<Class<?>> extractType(FROM source) {
+        if (Objects.isNull(source)) {
+            LOG.trace("Cannot extract type from source, because it is null");
+            return Optional.empty();
+        }
+
+        Class<?> type = source.getClass();
+
+        if (source instanceof Collection) {
+            final Optional first = ((Collection) source).stream().findFirst();
+
+            if (!first.isPresent()) {
+                LOG.trace("Cannot extract type {}, because it is empty collection", type);
+                return Optional.empty();
+            }
+
+            type = first.get().getClass();
         }
 
-        final Object result = convertor.convert(source, data);
-        return Optional.of((TO) result);
+        return Optional.of(type);
     }
 }
\ No newline at end of file

diff --git a/openflowplugin/src/main/java/org/opendaylight/openflowplugin/openflow/md/core/sal/convertor/ConvertorRegistrator.java b/openflowplugin/src/main/java/org/opendaylight/openflowplugin/openflow/md/core/sal/convertor/ConvertorRegistrator.java
new file mode 100644 (file)
index 0000000..bc889dc
--- /dev/null
+++ b/openflowplugin/src/main/java/org/opendaylight/openflowplugin/openflow/md/core/sal/convertor/ConvertorRegistrator.java
@@ -0,0 +1,30 @@
+/*
+ * Copyright (c) 2016 Cisco Systems, Inc. and others.  All rights reserved.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License v1.0 which accompanies this distribution,
+ * and is available at http://www.eclipse.org/legal/epl-v10.html
+ */
+
+package org.opendaylight.openflowplugin.openflow.md.core.sal.convertor;
+
+import org.opendaylight.openflowplugin.openflow.md.core.sal.convertor.common.Convertor;
+import org.opendaylight.openflowplugin.openflow.md.core.sal.convertor.common.ParametrizedConvertor;
+
+public interface ConvertorRegistrator {
+    /**
+     * Register convertor.
+     *
+     * @param convertor the convertor
+     * @return previous convertor if already registered or null
+     */
+    Convertor registerConvertor(final Convertor convertor);
+
+    /**
+     * Register convertor.
+     *
+     * @param convertor the convertor
+     * @return previous convertor if already registered or null
+     */
+    ParametrizedConvertor registerConvertor(final ParametrizedConvertor convertor);
+}
\ No newline at end of file

diff --git a/openflowplugin/src/test/java/org/opendaylight/openflowplugin/openflow/md/core/sal/convertor/ConvertorManagerTest.java b/openflowplugin/src/test/java/org/opendaylight/openflowplugin/openflow/md/core/sal/convertor/ConvertorManagerTest.java
index df20423dc652186c4d739decc731ef5ff15ef340..a35f08516ef7c4f3cd8c8e53c928ee76da9b1071 100644 (file)
--- a/openflowplugin/src/test/java/org/opendaylight/openflowplugin/openflow/md/core/sal/convertor/ConvertorManagerTest.java
+++ b/openflowplugin/src/test/java/org/opendaylight/openflowplugin/openflow/md/core/sal/convertor/ConvertorManagerTest.java
@@ -10,6 +10,7 @@ package org.opendaylight.openflowplugin.openflow.md.core.sal.convertor;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 
 import java.util.Collections;
@@ -63,14 +64,14 @@ public class ConvertorManagerTest {
 
     @Test
     public void testRegisterConvertor() throws Exception {
-        boolean success = ConvertorManager.getInstance().registerConvertor(convertor);
-        assertFalse("Convertor should be already registered", success);
+        Convertor result = ConvertorManager.getInstance().registerConvertor(convertor);
+        assertNotNull("Convertor should be already registered", result);
     }
 
     @Test
     public void testRegisterParametrizedConvertor() throws Exception {
-        boolean success = ConvertorManager.getInstance().registerConvertor(parametrizedConvertor);
-        assertFalse("Parametrized convertor should be already registered", success);
+        ParametrizedConvertor result = ConvertorManager.getInstance().registerConvertor(parametrizedConvertor);
+        assertNotNull("Parametrized convertor should be already registered", result);
     }
 
     @Test