From 9572e129f8bcc15c1d14cdd7b40dabbaa26d960e Mon Sep 17 00:00:00 2001 From: Andrzej Bialecki Date: Thu, 16 Aug 2018 16:59:12 +0200 Subject: [PATCH] SOLR-12668: Autoscaling trigger listeners should be executed in the order of their creation. --- solr/CHANGES.txt | 2 + .../autoscaling/TriggerIntegrationTest.java | 23 ++++++++++- .../sim/TestTriggerIntegration.java | 22 ++++++++++- .../cloud/autoscaling/AutoScalingConfig.java | 38 +++++++++---------- 4 files changed, 64 insertions(+), 21 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 3885761baf3..7f851f4f6e6 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -241,6 +241,8 @@ Bug Fixes * SOLR-12670: RecoveryStrategy logs wrong wait time when retrying recovery. (shalin) +* SOLR-12668: Autoscaling trigger listeners should be executed in the order of their creation. (ab) + Optimizations ---------------------- diff --git a/solr/core/src/test/org/apache/solr/cloud/autoscaling/TriggerIntegrationTest.java b/solr/core/src/test/org/apache/solr/cloud/autoscaling/TriggerIntegrationTest.java index 454a935fb5e..53499364e8a 100644 --- a/solr/core/src/test/org/apache/solr/cloud/autoscaling/TriggerIntegrationTest.java +++ b/solr/core/src/test/org/apache/solr/cloud/autoscaling/TriggerIntegrationTest.java @@ -500,6 +500,7 @@ public class TriggerIntegrationTest extends SolrCloudTestCase { } static Map> listenerEvents = new HashMap<>(); + static List allListenerEvents = new ArrayList<>(); static CountDownLatch listenerCreated = new CountDownLatch(1); static boolean failDummyAction = false; @@ -514,7 +515,9 @@ public class TriggerIntegrationTest extends SolrCloudTestCase { public synchronized void onEvent(TriggerEvent event, TriggerEventProcessorStage stage, String actionName, ActionContext context, Throwable error, String message) { List lst = listenerEvents.computeIfAbsent(config.name, s -> new ArrayList<>()); - lst.add(new CapturedEvent(timeSource.getTimeNs(), context, config, stage, actionName, event, message)); + CapturedEvent ev = new CapturedEvent(timeSource.getTimeNs(), context, config, stage, actionName, event, message); + lst.add(ev); + allListenerEvents.add(ev); } } @@ -627,10 +630,28 @@ public class TriggerIntegrationTest extends SolrCloudTestCase { assertEquals(TriggerEventProcessorStage.SUCCEEDED, capturedEvents.get(3).stage); + // check global ordering of events (SOLR-12668) + int fooIdx = -1; + int barIdx = -1; + for (int i = 0; i < allListenerEvents.size(); i++) { + CapturedEvent ev = allListenerEvents.get(i); + if (ev.stage == TriggerEventProcessorStage.BEFORE_ACTION && ev.actionName.equals("test")) { + if (ev.config.name.equals("foo")) { + fooIdx = i; + } else if (ev.config.name.equals("bar")) { + barIdx = i; + } + } + } + assertTrue("fooIdx not found", fooIdx != -1); + assertTrue("barIdx not found", barIdx != -1); + assertTrue("foo fired later than bar: fooIdx=" + fooIdx + ", barIdx=" + barIdx, fooIdx < barIdx); + // reset triggerFired.set(false); triggerFiredLatch = new CountDownLatch(1); listenerEvents.clear(); + allListenerEvents.clear(); failDummyAction = true; newNode = cluster.startJettySolrRunner(); diff --git a/solr/core/src/test/org/apache/solr/cloud/autoscaling/sim/TestTriggerIntegration.java b/solr/core/src/test/org/apache/solr/cloud/autoscaling/sim/TestTriggerIntegration.java index b49a7d507fa..9f583640ec4 100644 --- a/solr/core/src/test/org/apache/solr/cloud/autoscaling/sim/TestTriggerIntegration.java +++ b/solr/core/src/test/org/apache/solr/cloud/autoscaling/sim/TestTriggerIntegration.java @@ -905,6 +905,7 @@ public class TestTriggerIntegration extends SimSolrCloudTestCase { } static Map> listenerEvents = new ConcurrentHashMap<>(); + static List allListenerEvents = new ArrayList<>(); static CountDownLatch listenerCreated = new CountDownLatch(1); static boolean failDummyAction = false; @@ -919,7 +920,9 @@ public class TestTriggerIntegration extends SimSolrCloudTestCase { public synchronized void onEvent(TriggerEvent event, TriggerEventProcessorStage stage, String actionName, ActionContext context, Throwable error, String message) { List lst = listenerEvents.computeIfAbsent(config.name, s -> new ArrayList<>()); - lst.add(new CapturedEvent(cluster.getTimeSource().getTimeNs(), context, config, stage, actionName, event, message)); + CapturedEvent ev = new CapturedEvent(cluster.getTimeSource().getTimeNs(), context, config, stage, actionName, event, message); + lst.add(ev); + allListenerEvents.add(ev); } } @@ -1033,6 +1036,23 @@ public class TestTriggerIntegration extends SimSolrCloudTestCase { assertEquals(TriggerEventProcessorStage.SUCCEEDED, testEvents.get(3).stage); + // check global ordering of events (SOLR-12668) + int fooIdx = -1; + int barIdx = -1; + for (int i = 0; i < allListenerEvents.size(); i++) { + CapturedEvent ev = allListenerEvents.get(i); + if (ev.stage == TriggerEventProcessorStage.BEFORE_ACTION && ev.actionName.equals("test")) { + if (ev.config.name.equals("foo")) { + fooIdx = i; + } else if (ev.config.name.equals("bar")) { + barIdx = i; + } + } + } + assertTrue("fooIdx not found", fooIdx != -1); + assertTrue("barIdx not found", barIdx != -1); + assertTrue("foo fired later than bar: fooIdx=" + fooIdx + ", barIdx=" + barIdx, fooIdx < barIdx); + // reset triggerFired.set(false); triggerFiredLatch = new CountDownLatch(1); diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/AutoScalingConfig.java b/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/AutoScalingConfig.java index 6a7187594b1..fa0505ee54f 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/AutoScalingConfig.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/AutoScalingConfig.java @@ -22,8 +22,8 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.EnumSet; -import java.util.HashMap; -import java.util.HashSet; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Locale; import java.util.Map; @@ -72,7 +72,7 @@ public class AutoScalingConfig implements MapWriter { if (properties == null) { this.properties = Collections.emptyMap(); } else { - this.properties = Collections.unmodifiableMap(new HashMap<>(properties)); + this.properties = Collections.unmodifiableMap(new LinkedHashMap<>(properties)); } trigger = (String)this.properties.get(AutoScalingParams.TRIGGER); List stageNames = getList(AutoScalingParams.STAGE, this.properties); @@ -85,10 +85,10 @@ public class AutoScalingConfig implements MapWriter { } } listenerClass = (String)this.properties.get(AutoScalingParams.CLASS); - Set bActions = new HashSet<>(); + Set bActions = new LinkedHashSet<>(); getList(AutoScalingParams.BEFORE_ACTION, this.properties).forEach(o -> bActions.add(String.valueOf(o))); beforeActions = Collections.unmodifiableSet(bActions); - Set aActions = new HashSet<>(); + Set aActions = new LinkedHashSet<>(); getList(AutoScalingParams.AFTER_ACTION, this.properties).forEach(o -> aActions.add(String.valueOf(o))); afterActions = Collections.unmodifiableSet(aActions); } @@ -161,7 +161,7 @@ public class AutoScalingConfig implements MapWriter { public TriggerConfig(String name, Map properties) { this.name = name; if (properties != null) { - this.properties = Collections.unmodifiableMap(new HashMap<>(properties)); + this.properties = Collections.unmodifiableMap(new LinkedHashMap<>(properties)); } else { this.properties = Collections.emptyMap(); } @@ -196,7 +196,7 @@ public class AutoScalingConfig implements MapWriter { * @return modified copy of the configuration */ public TriggerConfig withEnabled(boolean enabled) { - Map props = new HashMap<>(properties); + Map props = new LinkedHashMap<>(properties); props.put(AutoScalingParams.ENABLED, String.valueOf(enabled)); return new TriggerConfig(name, props); } @@ -208,7 +208,7 @@ public class AutoScalingConfig implements MapWriter { * @return modified copy of the configuration */ public TriggerConfig withProperty(String key, Object value) { - Map props = new HashMap<>(properties); + Map props = new LinkedHashMap<>(properties); props.put(key, String.valueOf(value)); return new TriggerConfig(name, props); } @@ -264,7 +264,7 @@ public class AutoScalingConfig implements MapWriter { */ public ActionConfig(Map properties) { if (properties != null) { - this.properties = Collections.unmodifiableMap(new HashMap<>(properties)); + this.properties = Collections.unmodifiableMap(new LinkedHashMap<>(properties)); } else { this.properties = Collections.emptyMap(); } @@ -327,10 +327,10 @@ public class AutoScalingConfig implements MapWriter { private AutoScalingConfig(Policy policy, Map triggerConfigs, Map listenerConfigs, Map properties, int zkVersion) { this.policy = policy; - this.triggers = triggerConfigs != null ? Collections.unmodifiableMap(triggerConfigs) : null; - this.listeners = listenerConfigs != null ? Collections.unmodifiableMap(listenerConfigs) : null; + this.triggers = triggerConfigs != null ? Collections.unmodifiableMap(new LinkedHashMap<>(triggerConfigs)) : null; + this.listeners = listenerConfigs != null ? Collections.unmodifiableMap(new LinkedHashMap<>(listenerConfigs)) : null; this.jsonMap = null; - this.properties = properties != null ? Collections.unmodifiableMap(properties) : null; + this.properties = properties != null ? Collections.unmodifiableMap(new LinkedHashMap<>(properties)) : null; this.zkVersion = zkVersion; this.empty = policy == null && (triggerConfigs == null || triggerConfigs.isEmpty()) && @@ -368,7 +368,7 @@ public class AutoScalingConfig implements MapWriter { if (trigMap == null) { triggers = Collections.emptyMap(); } else { - HashMap newTriggers = new HashMap<>(trigMap.size()); + Map newTriggers = new LinkedHashMap<>(trigMap.size()); for (Map.Entry entry : trigMap.entrySet()) { newTriggers.put(entry.getKey(), new TriggerConfig(entry.getKey(), (Map)entry.getValue())); } @@ -411,7 +411,7 @@ public class AutoScalingConfig implements MapWriter { if (map == null) { listeners = Collections.emptyMap(); } else { - HashMap newListeners = new HashMap<>(map.size()); + Map newListeners = new LinkedHashMap<>(map.size()); for (Map.Entry entry : map.entrySet()) { newListeners.put(entry.getKey(), new TriggerListenerConfig(entry.getKey(), (Map)entry.getValue())); } @@ -431,7 +431,7 @@ public class AutoScalingConfig implements MapWriter { if (map == null) { this.properties = Collections.emptyMap(); } else { - this.properties = new HashMap<>(map); + this.properties = new LinkedHashMap<>(map); } } else { this.properties = Collections.emptyMap(); @@ -473,7 +473,7 @@ public class AutoScalingConfig implements MapWriter { * @return modified copy of the configuration */ public AutoScalingConfig withTriggerConfig(TriggerConfig config) { - Map configs = new HashMap<>(getTriggerConfigs()); + Map configs = new LinkedHashMap<>(getTriggerConfigs()); configs.put(config.name, config); return withTriggerConfigs(configs); } @@ -484,7 +484,7 @@ public class AutoScalingConfig implements MapWriter { * @return modified copy of the configuration, even if the specified config name didn't exist. */ public AutoScalingConfig withoutTriggerConfig(String name) { - Map configs = new HashMap<>(getTriggerConfigs()); + Map configs = new LinkedHashMap<>(getTriggerConfigs()); configs.remove(name); return withTriggerConfigs(configs); } @@ -504,7 +504,7 @@ public class AutoScalingConfig implements MapWriter { * @return modified copy of the configuration */ public AutoScalingConfig withTriggerListenerConfig(TriggerListenerConfig config) { - Map configs = new HashMap<>(getTriggerListenerConfigs()); + Map configs = new LinkedHashMap<>(getTriggerListenerConfigs()); configs.put(config.name, config); return withTriggerListenerConfigs(configs); } @@ -515,7 +515,7 @@ public class AutoScalingConfig implements MapWriter { * @return modified copy of the configuration, even if the specified config name didn't exist. */ public AutoScalingConfig withoutTriggerListenerConfig(String name) { - Map configs = new HashMap<>(getTriggerListenerConfigs()); + Map configs = new LinkedHashMap<>(getTriggerListenerConfigs()); configs.remove(name); return withTriggerListenerConfigs(configs); }