Clarify config pusher message 86/51786/2
authorRobert Varga <rovarga@cisco.com>
Mon, 13 Feb 2017 13:53:20 +0000 (14:53 +0100)
committerRobert Varga <rovarga@cisco.com>
Mon, 13 Feb 2017 20:35:23 +0000 (21:35 +0100)
Reword the message logged so it is more obvious what is going
on. Also lower it from warning to info and perform a general code
cleanup.

Change-Id: I37035e8df15b10f40568c3e894ffe2875b935d95
Signed-off-by: Robert Varga <rovarga@cisco.com>
opendaylight/config/config-persister-feature4-adapter/src/main/java/org/opendaylight/controller/configpusherfeature/internal/FeatureConfigPusher.java

index 7f254d5..ed1b620 100644 (file)
@@ -11,13 +11,14 @@ import com.google.common.base.Optional;
 import com.google.common.collect.LinkedHashMultimap;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Set;
+import java.util.concurrent.TimeUnit;
 import org.apache.karaf.features.Feature;
 import org.apache.karaf.features.FeaturesService;
 import org.opendaylight.controller.config.persist.api.ConfigPusher;
-import org.opendaylight.controller.config.persist.api.ConfigSnapshotHolder;
 import org.opendaylight.controller.config.persist.storage.file.xml.XmlFileStorageAdapter;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -27,22 +28,25 @@ import org.slf4j.LoggerFactory;
  */
 public class FeatureConfigPusher {
     private static final Logger LOG = LoggerFactory.getLogger(FeatureConfigPusher.class);
-    private static final int MAX_RETRIES=100;
-    private static final int RETRY_PAUSE_MILLIS=1;
+    private static final int MAX_RETRIES = 100;
+    private static final int RETRY_PAUSE_MILLIS = 1;
+
     private FeaturesService featuresService = null;
     private ConfigPusher pusher = null;
+
     /*
      * A LinkedHashSet (to preserve order and insure uniqueness) of the pushedConfigs
      * This is used to prevent pushing duplicate configs if a Feature is in multiple dependency
      * chains.  Also, preserves the *original* Feature chain for which we pushed the config.
      * (which is handy for logging).
      */
-    Set<FeatureConfigSnapshotHolder> pushedConfigs = new LinkedHashSet<>();
+    private final Set<FeatureConfigSnapshotHolder> pushedConfigs = new LinkedHashSet<>();
+
     /*
      * LinkedHashMultimap to track which configs we pushed for each Feature installation
      * For future use
      */
-    LinkedHashMultimap<Feature,FeatureConfigSnapshotHolder> feature2configs = LinkedHashMultimap.create();
+    private final LinkedHashMultimap<Feature,FeatureConfigSnapshotHolder> feature2configs = LinkedHashMultimap.create();
 
     /*
      * @param p - ConfigPusher to push ConfigSnapshotHolders
@@ -60,11 +64,10 @@ public class FeatureConfigPusher {
      * If a Feature is not in the returned LinkedHashMultimap then we couldn't push its configs
      * (Ususally because it was not yet installed)
      */
-    public LinkedHashMultimap<Feature, FeatureConfigSnapshotHolder> pushConfigs(final List<Feature> features) throws Exception {
+    public LinkedHashMultimap<Feature, FeatureConfigSnapshotHolder> pushConfigs(final List<Feature> features)
+            throws Exception {
         LinkedHashMultimap<Feature, FeatureConfigSnapshotHolder> pushedFeatures = LinkedHashMultimap.create();
         for (Feature feature : features) {
-
-
             Set<FeatureConfigSnapshotHolder> configSnapShots = pushConfig(feature);
             if (!configSnapShots.isEmpty()) {
                 pushedFeatures.putAll(feature, configSnapShots);
@@ -74,50 +77,55 @@ public class FeatureConfigPusher {
     }
 
     private Set<FeatureConfigSnapshotHolder> pushConfig(final Feature feature) throws Exception {
-        Set<FeatureConfigSnapshotHolder> configs = new LinkedHashSet<>();
         // Ignore feature conditions — these encode conditions on other features and shouldn't be processed here
         if (feature.getName().contains("-condition-")) {
             LOG.debug("Ignoring conditional feature {}", feature);
-        } else if (isInstalled(feature)) {
-            // FIXME Workaround for BUG-2836, features service returns null for feature: standard-condition-webconsole_0_0_0, 3.0.1
-            if (featuresService.getFeature(feature.getName(), feature.getVersion()) == null) {
-                LOG.debug("Feature: {}, {} is missing from features service. Skipping", feature.getName(), feature.getVersion());
-            } else {
-                ChildAwareFeatureWrapper wrappedFeature = new ChildAwareFeatureWrapper(feature, featuresService);
-                configs = wrappedFeature.getFeatureConfigSnapshotHolders();
-                if (!configs.isEmpty()) {
-                    configs = pushConfig(configs, feature);
-                    feature2configs.putAll(feature, configs);
-                }
-            }
+            return Collections.emptySet();
+        }
+
+        if (!isInstalled(feature)) {
+            return Collections.emptySet();
+        }
+        // FIXME Workaround for BUG-2836, features service returns null for feature:
+        // standard-condition-webconsole_0_0_0, 3.0.1
+        if (featuresService.getFeature(feature.getName(), feature.getVersion()) == null) {
+            LOG.debug("Feature: {}, {} is missing from features service. Skipping", feature.getName(),
+                feature.getVersion());
+            return Collections.emptySet();
+        }
+
+        ChildAwareFeatureWrapper wrappedFeature = new ChildAwareFeatureWrapper(feature, featuresService);
+        Set<FeatureConfigSnapshotHolder> configs = wrappedFeature.getFeatureConfigSnapshotHolders();
+        if (!configs.isEmpty()) {
+            configs = pushConfig(configs, feature);
+            feature2configs.putAll(feature, configs);
         }
         return configs;
     }
 
-    private boolean isInstalled(final Feature feature) {
+    private boolean isInstalled(final Feature feature) throws InterruptedException {
         for (int retries = 0; retries < MAX_RETRIES; retries++) {
             try {
                 List<Feature> installedFeatures = Arrays.asList(featuresService.listInstalledFeatures());
-                if(installedFeatures.contains(feature)) {
+                if (installedFeatures.contains(feature)) {
                     return true;
-                } else {
-                    LOG.warn("Karaf featuresService.listInstalledFeatures() has not yet finished installing feature (retry {}) {} {}",retries,feature.getName(),feature.getVersion());
                 }
+
+                LOG.info("Karaf Feature Service has not yet finished installing feature {}/{} (retry {})",
+                    feature.getName(), feature.getVersion(), retries);
             } catch (Exception e) {
                 LOG.warn("Karaf featuresService.listInstalledFeatures() has thrown an exception, retry {}", retries, e);
             }
-            try {
-                Thread.sleep(RETRY_PAUSE_MILLIS);
-            } catch (InterruptedException e1) {
-                throw new IllegalStateException(e1);
-            }
+
+            TimeUnit.MILLISECONDS.sleep(RETRY_PAUSE_MILLIS);
         }
-        LOG.error("Giving up (after {} retries) on Karaf featuresService.listInstalledFeatures() which has not yet finished installing feature {} {}",MAX_RETRIES,feature.getName(),feature.getVersion());
+        LOG.error("Giving up (after {} retries) on Karaf featuresService.listInstalledFeatures() which has not yet finished installing feature {} {}",
+            MAX_RETRIES, feature.getName(), feature.getVersion());
         return false;
     }
 
-    private Set<FeatureConfigSnapshotHolder> pushConfig(final Set<FeatureConfigSnapshotHolder> configs, final Feature feature)
-            throws InterruptedException {
+    private Set<FeatureConfigSnapshotHolder> pushConfig(final Set<FeatureConfigSnapshotHolder> configs,
+            final Feature feature) throws InterruptedException {
         Set<FeatureConfigSnapshotHolder> configsToPush = new LinkedHashSet<>(configs);
         configsToPush.removeAll(pushedConfigs);
         if (!configsToPush.isEmpty()) {
@@ -129,7 +137,7 @@ public class FeatureConfigPusher {
                 LOG.warn("Ignoring default configuration {} for feature {}, the configuration is present in {}",
                         configsToPush, feature.getId(), currentCfgPusher.get());
             } else {
-                pusher.pushConfigs(new ArrayList<ConfigSnapshotHolder>(configsToPush));
+                pusher.pushConfigs(new ArrayList<>(configsToPush));
             }
 
             pushedConfigs.addAll(configsToPush);