From 6b6932e8e1f72caf29a078f0532a56c284711f9f Mon Sep 17 00:00:00 2001 From: Christine Poerschke Date: Wed, 10 Feb 2016 22:19:06 +0000 Subject: [PATCH] SOLR-8621: WrapperMergePolicyFactory logic tweaks * fix so that getMergePolicy() can now be called more than once * added WrapperMergePolicyFactoryTest.testUpgradeIndexMergePolicyFactory() * account for overlap between wrapping and wrapped setters (and disallow it) ** illustration: 0.24 mergePolicy TieredMergePolicyFactory 0.42 ** implementation details: the wrapping MP's setter calls the wrapped MP's setter and in the current code the wrapping MP's value prevails i.e. the 0.24 value in the illustration since the wrapped MP is constructed before the wrapping MP. an end-user however might reasonably assume that the wrapped MP's 0.42 value will prevail. at best configuring the same setter twice within the same overall element is ambiguous and so the code now disallows it. --- .../solr/index/WrapperMergePolicyFactory.java | 25 ++++++++--- .../index/WrapperMergePolicyFactoryTest.java | 43 +++++++++++++++++++ 2 files changed, 62 insertions(+), 6 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/index/WrapperMergePolicyFactory.java b/solr/core/src/java/org/apache/solr/index/WrapperMergePolicyFactory.java index 24ba2378159..61088a8dbc0 100644 --- a/solr/core/src/java/org/apache/solr/index/WrapperMergePolicyFactory.java +++ b/solr/core/src/java/org/apache/solr/index/WrapperMergePolicyFactory.java @@ -16,7 +16,9 @@ */ package org.apache.solr.index; +import java.util.HashSet; import java.util.Iterator; +import java.util.Set; import org.apache.lucene.index.MergePolicy; import org.apache.solr.core.SolrResourceLoader; @@ -34,10 +36,26 @@ public abstract class WrapperMergePolicyFactory extends MergePolicyFactory { static final String WRAPPED_PREFIX = "wrapped.prefix"; // not private so that test(s) can use it private final MergePolicyFactoryArgs wrappedMergePolicyArgs; + private final String wrappedMergePolicyClassName; protected WrapperMergePolicyFactory(SolrResourceLoader resourceLoader, MergePolicyFactoryArgs args, IndexSchema schema) { super(resourceLoader, args, schema); wrappedMergePolicyArgs = filterWrappedMergePolicyFactoryArgs(); + if (wrappedMergePolicyArgs == null) { + wrappedMergePolicyClassName = null; + } else { + wrappedMergePolicyClassName = (String) wrappedMergePolicyArgs.remove(CLASS); + if (wrappedMergePolicyClassName == null) { + throw new IllegalArgumentException("Class name not defined for wrapped MergePolicyFactory!"); + } + } + if (wrappedMergePolicyArgs != null) { + final Set overshadowedWrappedMergePolicyArgs = new HashSet<>(wrappedMergePolicyArgs.keys()); + overshadowedWrappedMergePolicyArgs.retainAll(args.keys()); + if (!overshadowedWrappedMergePolicyArgs.isEmpty()) { + throw new IllegalArgumentException("Wrapping and wrapped merge policy args overlap! "+overshadowedWrappedMergePolicyArgs); + } + } } /** @@ -55,13 +73,8 @@ public abstract class WrapperMergePolicyFactory extends MergePolicyFactory { return getDefaultWrappedMergePolicy(); } - final String className = (String) wrappedMergePolicyArgs.remove(CLASS); - if (className == null) { - throw new IllegalArgumentException("Class name not defined for wrapped MergePolicyFactory!"); - } - final MergePolicyFactory mpf = resourceLoader.newInstance( - className, + wrappedMergePolicyClassName, MergePolicyFactory.class, NO_SUB_PACKAGES, new Class[] {SolrResourceLoader.class, MergePolicyFactoryArgs.class, IndexSchema.class}, diff --git a/solr/core/src/test/org/apache/solr/index/WrapperMergePolicyFactoryTest.java b/solr/core/src/test/org/apache/solr/index/WrapperMergePolicyFactoryTest.java index cd904f5455f..e4c7b3dc83e 100644 --- a/solr/core/src/test/org/apache/solr/index/WrapperMergePolicyFactoryTest.java +++ b/solr/core/src/test/org/apache/solr/index/WrapperMergePolicyFactoryTest.java @@ -19,6 +19,7 @@ package org.apache.solr.index; import org.apache.lucene.index.MergePolicy; import org.apache.lucene.index.NoMergePolicy; import org.apache.lucene.index.TieredMergePolicy; +import org.apache.lucene.index.UpgradeIndexMergePolicy; import org.apache.solr.SolrTestCaseJ4; import org.apache.solr.core.SolrResourceLoader; import org.apache.solr.schema.IndexSchema; @@ -68,6 +69,48 @@ public class WrapperMergePolicyFactoryTest extends SolrTestCaseJ4 { assertEquals("maxMergedSegmentMB", testMaxMergedSegmentMB, tmp.getMaxMergedSegmentMB(), 0.0d); } + public void testUpgradeIndexMergePolicyFactory() { + final int N = 10; + final Double wrappingNoCFSRatio = random().nextBoolean() ? null : random().nextInt(N+1)/((double)N); // must be: 0.0 <= value <= 1.0 + final Double wrappedNoCFSRatio = random().nextBoolean() ? null : random().nextInt(N+1)/((double)N); // must be: 0.0 <= value <= 1.0 + implTestUpgradeIndexMergePolicyFactory(wrappingNoCFSRatio, wrappedNoCFSRatio); + } + + private void implTestUpgradeIndexMergePolicyFactory(Double wrappingNoCFSRatio, Double wrappedNoCFSRatio) { + final MergePolicyFactoryArgs args = new MergePolicyFactoryArgs(); + if (wrappingNoCFSRatio != null) { + args.add("noCFSRatio", wrappingNoCFSRatio); // noCFSRatio for the wrapping merge policy + } + args.add(WrapperMergePolicyFactory.WRAPPED_PREFIX, "wrapped"); + args.add("wrapped.class", TieredMergePolicyFactory.class.getName()); + if (wrappedNoCFSRatio != null) { + args.add("wrapped.noCFSRatio", wrappedNoCFSRatio); // noCFSRatio for the wrapped merge policy + } + + MergePolicyFactory mpf; + try { + mpf = new UpgradeIndexMergePolicyFactory(resourceLoader, args, null); + assertFalse("Should only reach here if wrapping and wrapped args don't overlap!", + (wrappingNoCFSRatio != null && wrappedNoCFSRatio != null)); + + for (int ii=1; ii<=2; ++ii) { // it should be okay to call getMergePolicy() more than once + final MergePolicy mp = mpf.getMergePolicy(); + if (wrappingNoCFSRatio != null) { + assertEquals("#"+ii+" wrappingNoCFSRatio", wrappingNoCFSRatio.doubleValue(), mp.getNoCFSRatio(), 0.0d); + } + if (wrappedNoCFSRatio != null) { + assertEquals("#"+ii+" wrappedNoCFSRatio", wrappedNoCFSRatio.doubleValue(), mp.getNoCFSRatio(), 0.0d); + } + assertSame(mp.getClass(), UpgradeIndexMergePolicy.class); + } + + } catch (IllegalArgumentException iae) { + assertEquals("Wrapping and wrapped merge policy args overlap! [noCFSRatio]", iae.getMessage()); + assertTrue("Should only reach here if wrapping and wrapped args do overlap!", + (wrappingNoCFSRatio != null && wrappedNoCFSRatio != null)); + } + } + private static class DefaultingWrapperMergePolicyFactory extends WrapperMergePolicyFactory { DefaultingWrapperMergePolicyFactory(SolrResourceLoader resourceLoader, MergePolicyFactoryArgs wrapperArgs, IndexSchema schema) {