From 0f10b5f0424d592ed319e1b90d91f28c3a8625a9 Mon Sep 17 00:00:00 2001 From: Andrzej Bialecki Date: Fri, 13 Mar 2020 12:30:17 +0100 Subject: [PATCH] SOLR-13264: IndexSizeTrigger aboveOp / belowOp properties not in valid properties. --- solr/CHANGES.txt | 3 +- .../cloud/autoscaling/IndexSizeTrigger.java | 74 ++++++++++--------- .../solr/cloud/autoscaling/TriggerBase.java | 16 +++- .../autoscaling/IndexSizeTriggerTest.java | 19 +++++ 4 files changed, 75 insertions(+), 37 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 8afaae2fe2b..9343f299130 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -61,7 +61,8 @@ Optimizations Bug Fixes --------------------- -(No changes) +* SOLR-13264: IndexSizeTrigger aboveOp / belowOp properties not in valid properties. + (Christine Poerschke, ab) Other Changes --------------------- diff --git a/solr/core/src/java/org/apache/solr/cloud/autoscaling/IndexSizeTrigger.java b/solr/core/src/java/org/apache/solr/cloud/autoscaling/IndexSizeTrigger.java index f32669c7e8d..209d10ff1ee 100644 --- a/solr/core/src/java/org/apache/solr/cloud/autoscaling/IndexSizeTrigger.java +++ b/solr/core/src/java/org/apache/solr/cloud/autoscaling/IndexSizeTrigger.java @@ -60,6 +60,7 @@ import static org.apache.solr.client.solrj.cloud.autoscaling.Variable.Type.CORE_ public class IndexSizeTrigger extends TriggerBase { private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); + // configuration properties public static final String ABOVE_BYTES_PROP = "aboveBytes"; public static final String ABOVE_DOCS_PROP = "aboveDocs"; public static final String ABOVE_OP_PROP = "aboveOp"; @@ -72,14 +73,15 @@ public class IndexSizeTrigger extends TriggerBase { public static final String SPLIT_METHOD_PROP = CommonAdminParams.SPLIT_METHOD; public static final String SPLIT_BY_PREFIX = CommonAdminParams.SPLIT_BY_PREFIX; - public static final String BYTES_SIZE_PROP = "__bytes__"; - public static final String TOTAL_BYTES_SIZE_PROP = "__total_bytes__"; - public static final String DOCS_SIZE_PROP = "__docs__"; - public static final String MAX_DOC_PROP = "__maxDoc__"; - public static final String COMMIT_SIZE_PROP = "__commitBytes__"; - public static final String ABOVE_SIZE_PROP = "aboveSize"; - public static final String BELOW_SIZE_PROP = "belowSize"; - public static final String VIOLATION_PROP = "violationType"; + // event properties + public static final String BYTES_SIZE_KEY = "__bytes__"; + public static final String TOTAL_BYTES_SIZE_KEY = "__total_bytes__"; + public static final String DOCS_SIZE_KEY = "__docs__"; + public static final String MAX_DOC_KEY = "__maxDoc__"; + public static final String COMMIT_SIZE_KEY = "__commitBytes__"; + public static final String ABOVE_SIZE_KEY = "aboveSize"; + public static final String BELOW_SIZE_KEY = "belowSize"; + public static final String VIOLATION_KEY = "violationType"; public static final int DEFAULT_MAX_OPS = 10; @@ -98,8 +100,10 @@ public class IndexSizeTrigger extends TriggerBase { public IndexSizeTrigger(String name) { super(TriggerEventType.INDEXSIZE, name); TriggerUtils.validProperties(validProperties, - ABOVE_BYTES_PROP, ABOVE_DOCS_PROP, BELOW_BYTES_PROP, BELOW_DOCS_PROP, - COLLECTIONS_PROP, MAX_OPS_PROP, SPLIT_METHOD_PROP, SPLIT_FUZZ_PROP, SPLIT_BY_PREFIX); + ABOVE_BYTES_PROP, ABOVE_DOCS_PROP, ABOVE_OP_PROP, + BELOW_BYTES_PROP, BELOW_DOCS_PROP, BELOW_OP_PROP, + COLLECTIONS_PROP, MAX_OPS_PROP, + SPLIT_METHOD_PROP, SPLIT_FUZZ_PROP, SPLIT_BY_PREFIX); } @Override @@ -331,13 +335,13 @@ public class IndexSizeTrigger extends TriggerBase { ReplicaInfo currentInfo = currentSizes.computeIfAbsent(info.getCore(), k -> (ReplicaInfo)info.clone()); if (tag.contains("INDEX")) { - currentInfo.getVariables().put(TOTAL_BYTES_SIZE_PROP, ((Number) size).longValue()); + currentInfo.getVariables().put(TOTAL_BYTES_SIZE_KEY, ((Number) size).longValue()); } else if (tag.endsWith("SEARCHER.searcher.numDocs")) { - currentInfo.getVariables().put(DOCS_SIZE_PROP, ((Number) size).longValue()); + currentInfo.getVariables().put(DOCS_SIZE_KEY, ((Number) size).longValue()); } else if (tag.endsWith("SEARCHER.searcher.maxDoc")) { - currentInfo.getVariables().put(MAX_DOC_PROP, ((Number) size).longValue()); + currentInfo.getVariables().put(MAX_DOC_KEY, ((Number) size).longValue()); } else if (tag.endsWith("SEARCHER.searcher.indexCommitSize")) { - currentInfo.getVariables().put(COMMIT_SIZE_PROP, ((Number) size).longValue()); + currentInfo.getVariables().put(COMMIT_SIZE_KEY, ((Number) size).longValue()); } } }); @@ -358,25 +362,25 @@ public class IndexSizeTrigger extends TriggerBase { currentSizes.forEach((coreName, info) -> { // calculate estimated bytes - long maxDoc = (Long)info.getVariable(MAX_DOC_PROP); - long numDocs = (Long)info.getVariable(DOCS_SIZE_PROP); - long commitSize = (Long)info.getVariable(COMMIT_SIZE_PROP, 0L); + long maxDoc = (Long)info.getVariable(MAX_DOC_KEY); + long numDocs = (Long)info.getVariable(DOCS_SIZE_KEY); + long commitSize = (Long)info.getVariable(COMMIT_SIZE_KEY, 0L); if (commitSize <= 0) { - commitSize = (Long)info.getVariable(TOTAL_BYTES_SIZE_PROP); + commitSize = (Long)info.getVariable(TOTAL_BYTES_SIZE_KEY); } // calculate estimated size as a side-effect commitSize = estimatedSize(maxDoc, numDocs, commitSize); - info.getVariables().put(BYTES_SIZE_PROP, commitSize); + info.getVariables().put(BYTES_SIZE_KEY, commitSize); - if ((Long)info.getVariable(BYTES_SIZE_PROP) > aboveBytes || - (Long)info.getVariable(DOCS_SIZE_PROP) > aboveDocs) { + if ((Long)info.getVariable(BYTES_SIZE_KEY) > aboveBytes || + (Long)info.getVariable(DOCS_SIZE_KEY) > aboveDocs) { if (waitForElapsed(coreName, now, lastAboveEventMap)) { List infos = aboveSize.computeIfAbsent(info.getCollection(), c -> new ArrayList<>()); if (!infos.contains(info)) { - if ((Long)info.getVariable(BYTES_SIZE_PROP) > aboveBytes) { - info.getVariables().put(VIOLATION_PROP, ABOVE_BYTES_PROP); + if ((Long)info.getVariable(BYTES_SIZE_KEY) > aboveBytes) { + info.getVariables().put(VIOLATION_KEY, ABOVE_BYTES_PROP); } else { - info.getVariables().put(VIOLATION_PROP, ABOVE_DOCS_PROP); + info.getVariables().put(VIOLATION_KEY, ABOVE_DOCS_PROP); } infos.add(info); splittable.add(info.getName()); @@ -392,17 +396,17 @@ public class IndexSizeTrigger extends TriggerBase { Map> belowSize = new HashMap<>(); currentSizes.forEach((coreName, info) -> { - if (((Long)info.getVariable(BYTES_SIZE_PROP) < belowBytes || - (Long)info.getVariable(DOCS_SIZE_PROP) < belowDocs) && + if (((Long)info.getVariable(BYTES_SIZE_KEY) < belowBytes || + (Long)info.getVariable(DOCS_SIZE_KEY) < belowDocs) && // make sure we don't produce conflicting ops !splittable.contains(info.getName())) { if (waitForElapsed(coreName, now, lastBelowEventMap)) { List infos = belowSize.computeIfAbsent(info.getCollection(), c -> new ArrayList<>()); if (!infos.contains(info)) { - if ((Long)info.getVariable(BYTES_SIZE_PROP) < belowBytes) { - info.getVariables().put(VIOLATION_PROP, BELOW_BYTES_PROP); + if ((Long)info.getVariable(BYTES_SIZE_KEY) < belowBytes) { + info.getVariables().put(VIOLATION_KEY, BELOW_BYTES_PROP); } else { - info.getVariables().put(VIOLATION_PROP, BELOW_DOCS_PROP); + info.getVariables().put(VIOLATION_KEY, BELOW_DOCS_PROP); } infos.add(info); } @@ -429,7 +433,7 @@ public class IndexSizeTrigger extends TriggerBase { // sort by decreasing size to first split the largest ones // XXX see the comment below about using DOCS_SIZE_PROP in lieu of BYTES_SIZE_PROP replicas.sort((r1, r2) -> { - long delta = (Long) r1.getVariable(DOCS_SIZE_PROP) - (Long) r2.getVariable(DOCS_SIZE_PROP); + long delta = (Long) r1.getVariable(DOCS_SIZE_KEY) - (Long) r2.getVariable(DOCS_SIZE_KEY); if (delta > 0) { return -1; } else if (delta < 0) { @@ -471,7 +475,7 @@ public class IndexSizeTrigger extends TriggerBase { // then we should be sorting by BYTES_SIZE_PROP. However, since DOCS and BYTES are // loosely correlated it's simpler to sort just by docs (which better reflects the "too small" // condition than index size, due to possibly existing deleted docs that still occupy space) - long delta = (Long) r1.getVariable(DOCS_SIZE_PROP) - (Long) r2.getVariable(DOCS_SIZE_PROP); + long delta = (Long) r1.getVariable(DOCS_SIZE_KEY) - (Long) r2.getVariable(DOCS_SIZE_KEY); if (delta > 0) { return 1; } else if (delta < 0) { @@ -545,12 +549,12 @@ public class IndexSizeTrigger extends TriggerBase { // avoid passing very large amounts of data here - just use replica names TreeMap above = new TreeMap<>(); aboveSize.forEach((coll, replicas) -> - replicas.forEach(r -> above.put(r.getCore(), "docs=" + r.getVariable(DOCS_SIZE_PROP) + ", bytes=" + r.getVariable(BYTES_SIZE_PROP)))); - properties.put(ABOVE_SIZE_PROP, above); + replicas.forEach(r -> above.put(r.getCore(), "docs=" + r.getVariable(DOCS_SIZE_KEY) + ", bytes=" + r.getVariable(BYTES_SIZE_KEY)))); + properties.put(ABOVE_SIZE_KEY, above); TreeMap below = new TreeMap<>(); belowSize.forEach((coll, replicas) -> - replicas.forEach(r -> below.put(r.getCore(), "docs=" + r.getVariable(DOCS_SIZE_PROP) + ", bytes=" + r.getVariable(BYTES_SIZE_PROP)))); - properties.put(BELOW_SIZE_PROP, below); + replicas.forEach(r -> below.put(r.getCore(), "docs=" + r.getVariable(DOCS_SIZE_KEY) + ", bytes=" + r.getVariable(BYTES_SIZE_KEY)))); + properties.put(BELOW_SIZE_KEY, below); } } diff --git a/solr/core/src/java/org/apache/solr/cloud/autoscaling/TriggerBase.java b/solr/core/src/java/org/apache/solr/cloud/autoscaling/TriggerBase.java index 46fd8e59ed2..feedd45e101 100644 --- a/solr/core/src/java/org/apache/solr/cloud/autoscaling/TriggerBase.java +++ b/solr/core/src/java/org/apache/solr/cloud/autoscaling/TriggerBase.java @@ -81,10 +81,24 @@ public abstract class TriggerBase implements AutoScaling.Trigger { this.eventType = eventType; this.name = name; - // subclasses may modify this set to include other supported properties + // subclasses may further modify this set to include other supported properties TriggerUtils.validProperties(validProperties, "name", "class", "event", "enabled", "waitFor", "actions"); } + /** + * Return a set of valid property names supported by this trigger. + */ + public final Set getValidProperties() { + return Collections.unmodifiableSet(this.validProperties); + } + + /** + * Return a set of required property names supported by this trigger. + */ + public final Set getRequiredProperties() { + return Collections.unmodifiableSet(this.requiredProperties); + } + @Override public void configure(SolrResourceLoader loader, SolrCloudManager cloudManager, Map properties) throws TriggerValidationException { this.cloudManager = cloudManager; diff --git a/solr/core/src/test/org/apache/solr/cloud/autoscaling/IndexSizeTriggerTest.java b/solr/core/src/test/org/apache/solr/cloud/autoscaling/IndexSizeTriggerTest.java index 7ec3142c3a7..5ce3628ff4d 100644 --- a/solr/core/src/test/org/apache/solr/cloud/autoscaling/IndexSizeTriggerTest.java +++ b/solr/core/src/test/org/apache/solr/cloud/autoscaling/IndexSizeTriggerTest.java @@ -20,8 +20,10 @@ package org.apache.solr.cloud.autoscaling; import static org.apache.solr.common.cloud.ZkStateReader.SOLR_AUTOSCALING_CONF_PATH; import java.lang.invoke.MethodHandles; +import java.lang.reflect.Field; import java.util.ArrayList; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -737,6 +739,23 @@ public class IndexSizeTriggerTest extends SolrCloudTestCase { } } + // make sure all defined properties are added to valid properties (SOLR-13264) + @Test + public void testValidProperties() throws Exception { + + final Set propFields = new HashSet<>(); + + final TriggerBase trigger = new IndexSizeTrigger("index_size_trigger"); + for (final Field field : trigger.getClass().getFields()) { + if (field.getName().endsWith("_PROP")) { + propFields.add(field.get(trigger).toString()); + } + } + propFields.removeAll(trigger.getValidProperties()); + + assertTrue("Invalid _PROP constants: "+propFields.toString(), propFields.isEmpty()); + } + private Map createTriggerProps(long waitForSeconds) { Map props = new HashMap<>(); props.put("event", "indexSize");