From c13d3c664e004090d4c05b25ec8b0985e9468836 Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Thu, 28 Sep 2017 11:49:58 +0900 Subject: [PATCH 1/7] Lazy initialization of JavaScript functions --- .../data/input/impl/JavaScriptParseSpec.java | 13 ++++---- .../input/impl/JavaScriptParseSpecTest.java | 7 ++--- .../setup/JavaScriptWorkerSelectStrategy.java | 19 +++++++----- .../JavaScriptAggregatorFactory.java | 31 ++++++++----------- .../post/JavaScriptPostAggregator.java | 11 +++---- .../extraction/JavaScriptExtractionFn.java | 17 +++------- .../query/filter/JavaScriptDimFilter.java | 20 ++++-------- .../aggregation/JavaScriptAggregatorTest.java | 15 +++------ ...avaScriptTieredBrokerSelectorStrategy.java | 19 +++++++----- 9 files changed, 65 insertions(+), 87 deletions(-) diff --git a/api/src/main/java/io/druid/data/input/impl/JavaScriptParseSpec.java b/api/src/main/java/io/druid/data/input/impl/JavaScriptParseSpec.java index 499f61d5bfee..4925cbed096e 100644 --- a/api/src/main/java/io/druid/data/input/impl/JavaScriptParseSpec.java +++ b/api/src/main/java/io/druid/data/input/impl/JavaScriptParseSpec.java @@ -22,7 +22,7 @@ import com.fasterxml.jackson.annotation.JacksonInject; import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; -import io.druid.java.util.common.ISE; +import com.google.common.base.Preconditions; import io.druid.java.util.common.parsers.JavaScriptParser; import io.druid.java.util.common.parsers.Parser; import io.druid.js.JavaScriptConfig; @@ -36,6 +36,9 @@ public class JavaScriptParseSpec extends ParseSpec private final String function; private final JavaScriptConfig config; + // This variable is lazily initialized to avoid unnecessary JavaScript compilation during JSON serde + private JavaScriptParser parser; + @JsonCreator public JavaScriptParseSpec( @JsonProperty("timestampSpec") TimestampSpec timestampSpec, @@ -45,6 +48,7 @@ public JavaScriptParseSpec( ) { super(timestampSpec, dimensionsSpec); + Preconditions.checkState(config.isEnabled(), "JavaScript is disabled"); this.function = function; this.config = config; @@ -64,11 +68,8 @@ public void verify(List usedCols) @Override public Parser makeParser() { - if (!config.isEnabled()) { - throw new ISE("JavaScript is disabled"); - } - - return new JavaScriptParser(function); + parser = parser == null ? new JavaScriptParser(function) : parser; + return parser; } @Override diff --git a/api/src/test/java/io/druid/data/input/impl/JavaScriptParseSpecTest.java b/api/src/test/java/io/druid/data/input/impl/JavaScriptParseSpecTest.java index e9dcc196f3a0..66a4270db619 100644 --- a/api/src/test/java/io/druid/data/input/impl/JavaScriptParseSpecTest.java +++ b/api/src/test/java/io/druid/data/input/impl/JavaScriptParseSpecTest.java @@ -88,6 +88,9 @@ public void testMakeParser() @Test public void testMakeParserNotAllowed() { + expectedException.expect(IllegalStateException.class); + expectedException.expectMessage("JavaScript is disabled"); + final JavaScriptConfig config = new JavaScriptConfig(false); JavaScriptParseSpec spec = new JavaScriptParseSpec( new TimestampSpec("abc", "iso", null), @@ -95,9 +98,5 @@ public void testMakeParserNotAllowed() "abc", config ); - - expectedException.expect(IllegalStateException.class); - expectedException.expectMessage("JavaScript is disabled"); - spec.makeParser(); } } diff --git a/indexing-service/src/main/java/io/druid/indexing/overlord/setup/JavaScriptWorkerSelectStrategy.java b/indexing-service/src/main/java/io/druid/indexing/overlord/setup/JavaScriptWorkerSelectStrategy.java index cd0f487e11c0..e8a6fe73fcde 100644 --- a/indexing-service/src/main/java/io/druid/indexing/overlord/setup/JavaScriptWorkerSelectStrategy.java +++ b/indexing-service/src/main/java/io/druid/indexing/overlord/setup/JavaScriptWorkerSelectStrategy.java @@ -28,7 +28,6 @@ import io.druid.indexing.common.task.Task; import io.druid.indexing.overlord.ImmutableWorkerInfo; import io.druid.indexing.overlord.config.WorkerTaskRunnerConfig; -import io.druid.java.util.common.ISE; import io.druid.js.JavaScriptConfig; import javax.script.Compilable; @@ -44,9 +43,11 @@ public static interface SelectorFunction public String apply(WorkerTaskRunnerConfig config, ImmutableMap zkWorkers, Task task); } - private final SelectorFunction fnSelector; private final String function; + // This variable is lazily initialized to avoid unnecessary JavaScript compilation during JSON serde + private SelectorFunction fnSelector; + @JsonCreator public JavaScriptWorkerSelectStrategy( @JsonProperty("function") String fn, @@ -54,20 +55,21 @@ public JavaScriptWorkerSelectStrategy( ) { Preconditions.checkNotNull(fn, "function must not be null"); + Preconditions.checkState(config.isEnabled(), "JavaScript is disabled"); - if (!config.isEnabled()) { - throw new ISE("JavaScript is disabled"); - } + this.function = fn; + } + private SelectorFunction compileSelectorFunction() + { final ScriptEngine engine = new ScriptEngineManager().getEngineByName("javascript"); try { - ((Compilable) engine).compile("var apply = " + fn).eval(); + ((Compilable) engine).compile("var apply = " + function).eval(); + return ((Invocable) engine).getInterface(SelectorFunction.class); } catch (ScriptException e) { throw Throwables.propagate(e); } - this.function = fn; - this.fnSelector = ((Invocable) engine).getInterface(SelectorFunction.class); } @Override @@ -75,6 +77,7 @@ public ImmutableWorkerInfo findWorkerForTask( WorkerTaskRunnerConfig config, ImmutableMap zkWorkers, Task task ) { + fnSelector = fnSelector == null ? compileSelectorFunction() : fnSelector; String worker = fnSelector.apply(config, zkWorkers, task); return worker == null ? null : zkWorkers.get(worker); } diff --git a/processing/src/main/java/io/druid/query/aggregation/JavaScriptAggregatorFactory.java b/processing/src/main/java/io/druid/query/aggregation/JavaScriptAggregatorFactory.java index 10e438c189b7..869ce24603ba 100644 --- a/processing/src/main/java/io/druid/query/aggregation/JavaScriptAggregatorFactory.java +++ b/processing/src/main/java/io/druid/query/aggregation/JavaScriptAggregatorFactory.java @@ -28,7 +28,6 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import com.google.common.primitives.Doubles; -import io.druid.java.util.common.ISE; import io.druid.java.util.common.StringUtils; import io.druid.js.JavaScriptConfig; import io.druid.segment.ColumnSelectorFactory; @@ -58,7 +57,8 @@ public class JavaScriptAggregatorFactory extends AggregatorFactory private final String fnCombine; private final JavaScriptConfig config; - private final JavaScriptAggregator.ScriptAggregator compiledScript; + // This variable is lazily initialized to avoid unnecessary JavaScript compilation during JSON serde + private JavaScriptAggregator.ScriptAggregator compiledScript; @JsonCreator public JavaScriptAggregatorFactory( @@ -75,6 +75,7 @@ public JavaScriptAggregatorFactory( Preconditions.checkNotNull(fnAggregate, "Must have a valid, non-null fnAggregate"); Preconditions.checkNotNull(fnReset, "Must have a valid, non-null fnReset"); Preconditions.checkNotNull(fnCombine, "Must have a valid, non-null fnCombine"); + Preconditions.checkState(config.isEnabled(), "JavaScript is disabled"); this.name = name; this.fieldNames = fieldNames; @@ -83,17 +84,12 @@ public JavaScriptAggregatorFactory( this.fnReset = fnReset; this.fnCombine = fnCombine; this.config = config; - - if (config.isEnabled()) { - this.compiledScript = compileScript(fnAggregate, fnReset, fnCombine); - } else { - this.compiledScript = null; - } } @Override public Aggregator factorize(final ColumnSelectorFactory columnFactory) { + checkCompiledScript(); return new JavaScriptAggregator( Lists.transform( fieldNames, @@ -106,13 +102,14 @@ public ObjectColumnSelector apply(@Nullable String s) } } ), - getCompiledScript() + compiledScript ); } @Override public BufferAggregator factorizeBuffered(final ColumnSelectorFactory columnSelectorFactory) { + checkCompiledScript(); return new JavaScriptBufferAggregator( Lists.transform( fieldNames, @@ -125,7 +122,7 @@ public ObjectColumnSelector apply(@Nullable String s) } } ), - getCompiledScript() + compiledScript ); } @@ -138,7 +135,8 @@ public Comparator getComparator() @Override public Object combine(Object lhs, Object rhs) { - return getCompiledScript().combine(((Number) lhs).doubleValue(), ((Number) rhs).doubleValue()); + checkCompiledScript(); + return compiledScript.combine(((Number) lhs).doubleValue(), ((Number) rhs).doubleValue()); } @Override @@ -157,7 +155,8 @@ public void reset(ColumnValueSelector selector) @Override public void fold(ColumnValueSelector selector) { - combined = getCompiledScript().combine(combined, selector.getDouble()); + checkCompiledScript(); + combined = compiledScript.combine(combined, selector.getDouble()); } @Override @@ -300,13 +299,9 @@ public String toString() '}'; } - private JavaScriptAggregator.ScriptAggregator getCompiledScript() + private void checkCompiledScript() { - if (compiledScript == null) { - throw new ISE("JavaScript is disabled"); - } - - return compiledScript; + compiledScript = compiledScript == null ? compileScript(fnAggregate, fnReset, fnCombine) : compiledScript; } @VisibleForTesting diff --git a/processing/src/main/java/io/druid/query/aggregation/post/JavaScriptPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/post/JavaScriptPostAggregator.java index 0b285c3e8c02..5aae34206207 100644 --- a/processing/src/main/java/io/druid/query/aggregation/post/JavaScriptPostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/post/JavaScriptPostAggregator.java @@ -86,7 +86,8 @@ public double apply(Object[] args) private final List fieldNames; private final String function; - private final Function fn; + // This variable is lazily initialized to avoid unnecessary JavaScript compilation during JSON serde + private Function fn; @JsonCreator public JavaScriptPostAggregator( @@ -99,12 +100,11 @@ public JavaScriptPostAggregator( Preconditions.checkNotNull(name, "Must have a valid, non-null post-aggregator name"); Preconditions.checkNotNull(fieldNames, "Must have a valid, non-null fieldNames"); Preconditions.checkNotNull(function, "Must have a valid, non-null function"); - Preconditions.checkState(config.isEnabled(), "JavaScript is disabled."); + Preconditions.checkState(config.isEnabled(), "JavaScript is disabled"); this.name = name; this.fieldNames = fieldNames; this.function = function; - this.fn = compile(function); } @Override @@ -127,6 +127,7 @@ public Object compute(Map combinedAggregators) for (String field : fieldNames) { args[i++] = combinedAggregators.get(field); } + fn = fn == null ? compile(function) : fn; return fn.apply(args); } @@ -179,9 +180,6 @@ public boolean equals(Object o) if (!fieldNames.equals(that.fieldNames)) { return false; } - if (!fn.equals(that.fn)) { - return false; - } if (!function.equals(that.function)) { return false; } @@ -198,7 +196,6 @@ public int hashCode() int result = name.hashCode(); result = 31 * result + fieldNames.hashCode(); result = 31 * result + function.hashCode(); - result = 31 * result + fn.hashCode(); return result; } } diff --git a/processing/src/main/java/io/druid/query/extraction/JavaScriptExtractionFn.java b/processing/src/main/java/io/druid/query/extraction/JavaScriptExtractionFn.java index 51efd1368441..c4483c11eb20 100644 --- a/processing/src/main/java/io/druid/query/extraction/JavaScriptExtractionFn.java +++ b/processing/src/main/java/io/druid/query/extraction/JavaScriptExtractionFn.java @@ -25,7 +25,6 @@ import com.google.common.base.Function; import com.google.common.base.Preconditions; import com.google.common.base.Strings; -import io.druid.java.util.common.ISE; import io.druid.java.util.common.StringUtils; import io.druid.js.JavaScriptConfig; import org.mozilla.javascript.Context; @@ -67,9 +66,11 @@ public String apply(Object input) } private final String function; - private final Function fn; private final boolean injective; + // This variable is lazily initialized to avoid unnecessary JavaScript compilation during JSON serde + private Function fn; + @JsonCreator public JavaScriptExtractionFn( @JsonProperty("function") String function, @@ -78,15 +79,10 @@ public JavaScriptExtractionFn( ) { Preconditions.checkNotNull(function, "function must not be null"); + Preconditions.checkState(config.isEnabled(), "JavaScript is disabled"); this.function = function; this.injective = injective; - - if (config.isEnabled()) { - this.fn = compile(function); - } else { - this.fn = null; - } } @JsonProperty @@ -115,10 +111,7 @@ public byte[] getCacheKey() @Nullable public String apply(@Nullable Object value) { - if (fn == null) { - throw new ISE("JavaScript is disabled"); - } - + fn = fn == null ? compile(function) : fn; return Strings.emptyToNull(fn.apply(value)); } diff --git a/processing/src/main/java/io/druid/query/filter/JavaScriptDimFilter.java b/processing/src/main/java/io/druid/query/filter/JavaScriptDimFilter.java index 372edca3b04c..6fb8749259ab 100644 --- a/processing/src/main/java/io/druid/query/filter/JavaScriptDimFilter.java +++ b/processing/src/main/java/io/druid/query/filter/JavaScriptDimFilter.java @@ -25,7 +25,6 @@ import com.google.common.base.Preconditions; import com.google.common.base.Predicate; import com.google.common.collect.RangeSet; -import io.druid.java.util.common.ISE; import io.druid.java.util.common.StringUtils; import io.druid.js.JavaScriptConfig; import io.druid.query.extraction.ExtractionFn; @@ -41,9 +40,9 @@ public class JavaScriptDimFilter implements DimFilter private final String dimension; private final String function; private final ExtractionFn extractionFn; - private final JavaScriptConfig config; - private final JavaScriptPredicateFactory predicateFactory; + // This variable is lazily initialized to avoid unnecessary JavaScript compilation during JSON serde + private JavaScriptPredicateFactory predicateFactory; @JsonCreator public JavaScriptDimFilter( @@ -55,16 +54,10 @@ public JavaScriptDimFilter( { Preconditions.checkArgument(dimension != null, "dimension must not be null"); Preconditions.checkArgument(function != null, "function must not be null"); + Preconditions.checkState(config.isEnabled(), "JavaScript is disabled"); this.dimension = dimension; this.function = function; this.extractionFn = extractionFn; - this.config = config; - - if (config.isEnabled()) { - this.predicateFactory = new JavaScriptPredicateFactory(function, extractionFn); - } else { - this.predicateFactory = null; - } } @JsonProperty @@ -111,10 +104,9 @@ public DimFilter optimize() @Override public Filter toFilter() { - if (!config.isEnabled()) { - throw new ISE("JavaScript is disabled"); - } - + predicateFactory = predicateFactory == null ? + new JavaScriptPredicateFactory(function, extractionFn) : + predicateFactory; return new JavaScriptFilter(dimension, predicateFactory); } diff --git a/processing/src/test/java/io/druid/query/aggregation/JavaScriptAggregatorTest.java b/processing/src/test/java/io/druid/query/aggregation/JavaScriptAggregatorTest.java index ea46da64a2bc..302cd0b835fd 100644 --- a/processing/src/test/java/io/druid/query/aggregation/JavaScriptAggregatorTest.java +++ b/processing/src/test/java/io/druid/query/aggregation/JavaScriptAggregatorTest.java @@ -280,6 +280,9 @@ public void testAggregateStrings() @Test public void testJavaScriptDisabledFactorize() { + expectedException.expect(IllegalStateException.class); + expectedException.expectMessage("JavaScript is disabled"); + final JavaScriptAggregatorFactory factory = new JavaScriptAggregatorFactory( "foo", ImmutableList.of("foo"), @@ -288,16 +291,13 @@ public void testJavaScriptDisabledFactorize() scriptDoubleSum.get("fnCombine"), new JavaScriptConfig(false) ); - - expectedException.expect(IllegalStateException.class); - expectedException.expectMessage("JavaScript is disabled"); - factory.factorize(DUMMY_COLUMN_SELECTOR_FACTORY); - Assert.assertTrue(false); } @Test public void testJavaScriptDisabledFactorizeBuffered() { + expectedException.expect(IllegalStateException.class); + expectedException.expectMessage("JavaScript is disabled"); final JavaScriptAggregatorFactory factory = new JavaScriptAggregatorFactory( "foo", ImmutableList.of("foo"), @@ -306,11 +306,6 @@ public void testJavaScriptDisabledFactorizeBuffered() scriptDoubleSum.get("fnCombine"), new JavaScriptConfig(false) ); - - expectedException.expect(IllegalStateException.class); - expectedException.expectMessage("JavaScript is disabled"); - factory.factorizeBuffered(DUMMY_COLUMN_SELECTOR_FACTORY); - Assert.assertTrue(false); } public static void main(String... args) throws Exception diff --git a/server/src/main/java/io/druid/server/router/JavaScriptTieredBrokerSelectorStrategy.java b/server/src/main/java/io/druid/server/router/JavaScriptTieredBrokerSelectorStrategy.java index 8e4dd8c5ba18..87930255e8ba 100644 --- a/server/src/main/java/io/druid/server/router/JavaScriptTieredBrokerSelectorStrategy.java +++ b/server/src/main/java/io/druid/server/router/JavaScriptTieredBrokerSelectorStrategy.java @@ -24,7 +24,6 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.google.common.base.Optional; import com.google.common.base.Preconditions; -import io.druid.java.util.common.ISE; import io.druid.js.JavaScriptConfig; import io.druid.query.Query; @@ -41,9 +40,11 @@ public static interface SelectorFunction public String apply(TieredBrokerConfig config, Query query); } - private final SelectorFunction fnSelector; private final String function; + // This variable is lazily initialized to avoid unnecessary JavaScript compilation during JSON serde + private SelectorFunction fnSelector; + @JsonCreator public JavaScriptTieredBrokerSelectorStrategy( @JsonProperty("function") String fn, @@ -51,20 +52,21 @@ public JavaScriptTieredBrokerSelectorStrategy( ) { Preconditions.checkNotNull(fn, "function must not be null"); + Preconditions.checkState(config.isEnabled(), "JavaScript is disabled"); - if (!config.isEnabled()) { - throw new ISE("JavaScript is disabled"); - } + this.function = fn; + } + private SelectorFunction compileSelectorFunction() + { final ScriptEngine engine = new ScriptEngineManager().getEngineByName("javascript"); try { - ((Compilable) engine).compile("var apply = " + fn).eval(); + ((Compilable) engine).compile("var apply = " + function).eval(); + return ((Invocable) engine).getInterface(SelectorFunction.class); } catch (ScriptException e) { throw new RuntimeException(e); } - this.function = fn; - this.fnSelector = ((Invocable) engine).getInterface(SelectorFunction.class); } @Override @@ -72,6 +74,7 @@ public Optional getBrokerServiceName( TieredBrokerConfig config, Query query ) { + fnSelector = fnSelector == null ? compileSelectorFunction() : fnSelector; return Optional.fromNullable(fnSelector.apply(config, query)); } From 2dc680b54fa5af299df04e51f7f2ee3c17819a40 Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Thu, 28 Sep 2017 12:22:15 +0900 Subject: [PATCH 2/7] Fix test failure --- .../druid/query/extraction/JavaScriptExtractionFnTest.java | 7 ++----- .../io/druid/query/filter/JavaScriptDimFilterTest.java | 6 ++---- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/processing/src/test/java/io/druid/query/extraction/JavaScriptExtractionFnTest.java b/processing/src/test/java/io/druid/query/extraction/JavaScriptExtractionFnTest.java index dc4494dadbde..929e649c2750 100644 --- a/processing/src/test/java/io/druid/query/extraction/JavaScriptExtractionFnTest.java +++ b/processing/src/test/java/io/druid/query/extraction/JavaScriptExtractionFnTest.java @@ -64,13 +64,10 @@ public void testJavascriptSubstring() @Test public void testJavascriptNotAllowed() { - String function = "function(str) { return str.substring(0,3); }"; - ExtractionFn extractionFn = new JavaScriptExtractionFn(function, false, new JavaScriptConfig(false)); - expectedException.expect(IllegalStateException.class); expectedException.expectMessage("JavaScript is disabled"); - extractionFn.apply("hey"); - Assert.assertTrue(false); + String function = "function(str) { return str.substring(0,3); }"; + ExtractionFn extractionFn = new JavaScriptExtractionFn(function, false, new JavaScriptConfig(false)); } @Test diff --git a/processing/src/test/java/io/druid/query/filter/JavaScriptDimFilterTest.java b/processing/src/test/java/io/druid/query/filter/JavaScriptDimFilterTest.java index 5c43f2390282..659554bc9b70 100644 --- a/processing/src/test/java/io/druid/query/filter/JavaScriptDimFilterTest.java +++ b/processing/src/test/java/io/druid/query/filter/JavaScriptDimFilterTest.java @@ -98,11 +98,9 @@ public void testToFilter() @Test public void testToFilterNotAllowed() { - JavaScriptDimFilter javaScriptDimFilter = new JavaScriptDimFilter("dim", FN1, null, new JavaScriptConfig(false)); - expectedException.expect(IllegalStateException.class); expectedException.expectMessage("JavaScript is disabled"); - javaScriptDimFilter.toFilter(); - Assert.assertTrue(false); + JavaScriptDimFilter javaScriptDimFilter = new JavaScriptDimFilter("dim", FN1, null, new JavaScriptConfig(false)); + } } From 3000d143dc30ca03ca6e1c5174534ca5860172df Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Sat, 7 Oct 2017 17:58:18 +0900 Subject: [PATCH 3/7] Fix thread-safety and postpone js conf check --- .../data/input/impl/JavaScriptParseSpec.java | 4 +- .../data/input/impl/StringInputRowParser.java | 15 +++- .../input/impl/JavaScriptParseSpecTest.java | 7 +- .../input/impl/StringInputRowParserTest.java | 85 +++++++++++++++++++ .../input/thrift/ThriftInputRowParser.java | 9 +- .../thrift/ThriftInputRowParserTest.java | 38 +++++++++ .../protobuf/ProtobufInputRowParser.java | 10 ++- .../protobuf/ProtobufInputRowParserTest.java | 35 ++++++++ .../JavaScriptAggregatorFactory.java | 26 ++++-- .../post/JavaScriptPostAggregator.java | 26 +++++- .../extraction/JavaScriptExtractionFn.java | 23 ++++- .../query/filter/JavaScriptDimFilter.java | 25 +++++- .../aggregation/JavaScriptAggregatorTest.java | 15 ++-- .../post/JavaScriptPostAggregatorTest.java | 9 +- .../JavaScriptExtractionFnTest.java | 7 +- .../query/filter/JavaScriptDimFilterTest.java | 6 +- .../io/druid/segment/indexing/DataSchema.java | 14 ++- 17 files changed, 312 insertions(+), 42 deletions(-) create mode 100644 api/src/test/java/io/druid/data/input/impl/StringInputRowParserTest.java diff --git a/api/src/main/java/io/druid/data/input/impl/JavaScriptParseSpec.java b/api/src/main/java/io/druid/data/input/impl/JavaScriptParseSpec.java index 4925cbed096e..88b652783061 100644 --- a/api/src/main/java/io/druid/data/input/impl/JavaScriptParseSpec.java +++ b/api/src/main/java/io/druid/data/input/impl/JavaScriptParseSpec.java @@ -48,7 +48,6 @@ public JavaScriptParseSpec( ) { super(timestampSpec, dimensionsSpec); - Preconditions.checkState(config.isEnabled(), "JavaScript is disabled"); this.function = function; this.config = config; @@ -68,6 +67,9 @@ public void verify(List usedCols) @Override public Parser makeParser() { + // JavaScript configuration should be checked when it's actually used because someone might still want Druid + // nodes to be able to deserialize JavaScript-based objects even though JavaScript is disabled. + Preconditions.checkState(config.isEnabled(), "JavaScript is disabled"); parser = parser == null ? new JavaScriptParser(function) : parser; return parser; } diff --git a/api/src/main/java/io/druid/data/input/impl/StringInputRowParser.java b/api/src/main/java/io/druid/data/input/impl/StringInputRowParser.java index a640ef10ac41..ec673271f0a6 100644 --- a/api/src/main/java/io/druid/data/input/impl/StringInputRowParser.java +++ b/api/src/main/java/io/druid/data/input/impl/StringInputRowParser.java @@ -43,10 +43,10 @@ public class StringInputRowParser implements ByteBufferInputRowParser private final ParseSpec parseSpec; private final MapInputRowParser mapParser; - private final Parser parser; private final Charset charset; - private CharBuffer chars = null; + private Parser parser; + private CharBuffer chars; @JsonCreator public StringInputRowParser( @@ -56,7 +56,6 @@ public StringInputRowParser( { this.parseSpec = parseSpec; this.mapParser = new MapInputRowParser(parseSpec); - this.parser = parseSpec.makeParser(); if (encoding != null) { this.charset = Charset.forName(encoding); @@ -126,6 +125,11 @@ private Map buildStringKeyMap(ByteBuffer input) public void startFileFromBeginning() { + if (parser == null) { + // parser should be created when it is really used to avoid unnecessary initialization of the underlying + // parseSpec. + parser = parseSpec.makeParser(); + } parser.startFileFromBeginning(); } @@ -138,6 +142,11 @@ public InputRow parse(@Nullable String input) @Nullable private Map parseString(@Nullable String inputString) { + if (parser == null) { + // parser should be created when it is really used to avoid unnecessary initialization of the underlying + // parseSpec. + parser = parseSpec.makeParser(); + } return parser.parse(inputString); } diff --git a/api/src/test/java/io/druid/data/input/impl/JavaScriptParseSpecTest.java b/api/src/test/java/io/druid/data/input/impl/JavaScriptParseSpecTest.java index 66a4270db619..e9dcc196f3a0 100644 --- a/api/src/test/java/io/druid/data/input/impl/JavaScriptParseSpecTest.java +++ b/api/src/test/java/io/druid/data/input/impl/JavaScriptParseSpecTest.java @@ -88,9 +88,6 @@ public void testMakeParser() @Test public void testMakeParserNotAllowed() { - expectedException.expect(IllegalStateException.class); - expectedException.expectMessage("JavaScript is disabled"); - final JavaScriptConfig config = new JavaScriptConfig(false); JavaScriptParseSpec spec = new JavaScriptParseSpec( new TimestampSpec("abc", "iso", null), @@ -98,5 +95,9 @@ public void testMakeParserNotAllowed() "abc", config ); + + expectedException.expect(IllegalStateException.class); + expectedException.expectMessage("JavaScript is disabled"); + spec.makeParser(); } } diff --git a/api/src/test/java/io/druid/data/input/impl/StringInputRowParserTest.java b/api/src/test/java/io/druid/data/input/impl/StringInputRowParserTest.java new file mode 100644 index 000000000000..d532ba5fd403 --- /dev/null +++ b/api/src/test/java/io/druid/data/input/impl/StringInputRowParserTest.java @@ -0,0 +1,85 @@ +/* + * Licensed to Metamarkets Group Inc. (Metamarkets) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. Metamarkets licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package io.druid.data.input.impl; + +import com.google.common.collect.ImmutableList; +import io.druid.js.JavaScriptConfig; +import org.hamcrest.CoreMatchers; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; + +public class StringInputRowParserTest +{ + @Rule + public ExpectedException expectedException = ExpectedException.none(); + + @Test + public void testDisableJavaScript() + { + final JavaScriptParseSpec parseSpec = new JavaScriptParseSpec( + new TimestampSpec("timestamp", "auto", null), + new DimensionsSpec( + DimensionsSpec.getDefaultSchemas( + ImmutableList.of( + "dim1", + "dim2" + ) + ), + null, + null + ), + "func", + new JavaScriptConfig(false) + ); + final StringInputRowParser parser = new StringInputRowParser(parseSpec, "UTF-8"); + + expectedException.expect(CoreMatchers.instanceOf(IllegalStateException.class)); + expectedException.expectMessage("JavaScript is disabled"); + + parser.startFileFromBeginning(); + } + + @Test + public void testDisableJavaScript2() + { + final JavaScriptParseSpec parseSpec = new JavaScriptParseSpec( + new TimestampSpec("timestamp", "auto", null), + new DimensionsSpec( + DimensionsSpec.getDefaultSchemas( + ImmutableList.of( + "dim1", + "dim2" + ) + ), + null, + null + ), + "func", + new JavaScriptConfig(false) + ); + final StringInputRowParser parser = new StringInputRowParser(parseSpec, "UTF-8"); + + expectedException.expect(CoreMatchers.instanceOf(IllegalStateException.class)); + expectedException.expectMessage("JavaScript is disabled"); + + parser.parse(""); + } +} diff --git a/extensions-contrib/thrift-extensions/src/main/java/io/druid/data/input/thrift/ThriftInputRowParser.java b/extensions-contrib/thrift-extensions/src/main/java/io/druid/data/input/thrift/ThriftInputRowParser.java index 9f103bf3bb98..94a07d22ecfe 100644 --- a/extensions-contrib/thrift-extensions/src/main/java/io/druid/data/input/thrift/ThriftInputRowParser.java +++ b/extensions-contrib/thrift-extensions/src/main/java/io/druid/data/input/thrift/ThriftInputRowParser.java @@ -52,7 +52,7 @@ public class ThriftInputRowParser implements InputRowParser private final String jarPath; private final String thriftClassName; - final private Parser parser; + private Parser parser; volatile private Class thriftClass = null; @JsonCreator @@ -67,7 +67,6 @@ public ThriftInputRowParser( Preconditions.checkNotNull(thriftClassName, "thrift class name"); this.parseSpec = parseSpec; - parser = parseSpec.makeParser(); } public Class getThriftClass() @@ -92,6 +91,12 @@ public Class getThriftClass() @Override public InputRow parse(Object input) { + if (parser == null) { + // parser should be created when it is really used to avoid unnecessary initialization of the underlying + // parseSpec. + parser = parseSpec.makeParser(); + } + // There is a Parser check in phase 2 of mapreduce job, thrift jar may not present in peon side. // Place it this initialization in constructor will get ClassNotFoundException try { diff --git a/extensions-contrib/thrift-extensions/src/test/java/io/druid/data/input/thrift/ThriftInputRowParserTest.java b/extensions-contrib/thrift-extensions/src/test/java/io/druid/data/input/thrift/ThriftInputRowParserTest.java index 14cdaab09e55..c811bff64cbd 100644 --- a/extensions-contrib/thrift-extensions/src/test/java/io/druid/data/input/thrift/ThriftInputRowParserTest.java +++ b/extensions-contrib/thrift-extensions/src/test/java/io/druid/data/input/thrift/ThriftInputRowParserTest.java @@ -19,17 +19,20 @@ package io.druid.data.input.thrift; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import io.druid.data.input.InputRow; import io.druid.data.input.impl.DimensionSchema; import io.druid.data.input.impl.DimensionsSpec; import io.druid.data.input.impl.JSONParseSpec; +import io.druid.data.input.impl.JavaScriptParseSpec; import io.druid.data.input.impl.ParseSpec; import io.druid.data.input.impl.StringDimensionSchema; import io.druid.data.input.impl.TimestampSpec; import io.druid.java.util.common.parsers.JSONPathFieldSpec; import io.druid.java.util.common.parsers.JSONPathFieldType; import io.druid.java.util.common.parsers.JSONPathSpec; +import io.druid.js.JavaScriptConfig; import org.apache.commons.codec.binary.Base64; import org.apache.hadoop.io.BytesWritable; import org.apache.thrift.TException; @@ -37,8 +40,11 @@ import org.apache.thrift.protocol.TBinaryProtocol; import org.apache.thrift.protocol.TCompactProtocol; import org.apache.thrift.protocol.TJSONProtocol; +import org.hamcrest.CoreMatchers; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import java.nio.ByteBuffer; @@ -47,6 +53,8 @@ public class ThriftInputRowParserTest { + @Rule + public ExpectedException expectedException = ExpectedException.none(); private ParseSpec parseSpec; @@ -111,6 +119,36 @@ public void testParse() throws Exception serializationAndTest(parser, bytes); } + @Test + public void testDisableJavaScript() + { + final JavaScriptParseSpec parseSpec = new JavaScriptParseSpec( + new TimestampSpec("timestamp", "auto", null), + new DimensionsSpec( + DimensionsSpec.getDefaultSchemas( + ImmutableList.of( + "dim1", + "dim2" + ) + ), + null, + null + ), + "func", + new JavaScriptConfig(false) + ); + ThriftInputRowParser parser = new ThriftInputRowParser( + parseSpec, + "example/book.jar", + "io.druid.data.input.thrift.Book" + ); + + expectedException.expect(CoreMatchers.instanceOf(IllegalStateException.class)); + expectedException.expectMessage("JavaScript is disabled"); + + parser.parse(ByteBuffer.allocate(1)); + } + public void serializationAndTest(ThriftInputRowParser parser, byte[] bytes) throws TException { ByteBuffer buffer = ByteBuffer.wrap(bytes); diff --git a/extensions-core/protobuf-extensions/src/main/java/io/druid/data/input/protobuf/ProtobufInputRowParser.java b/extensions-core/protobuf-extensions/src/main/java/io/druid/data/input/protobuf/ProtobufInputRowParser.java index b039539df495..ec9910742345 100644 --- a/extensions-core/protobuf-extensions/src/main/java/io/druid/data/input/protobuf/ProtobufInputRowParser.java +++ b/extensions-core/protobuf-extensions/src/main/java/io/druid/data/input/protobuf/ProtobufInputRowParser.java @@ -47,10 +47,10 @@ public class ProtobufInputRowParser implements ByteBufferInputRowParser { private final ParseSpec parseSpec; - private Parser parser; private final String descriptorFilePath; private final String protoMessageType; - private Descriptor descriptor; + private final Descriptor descriptor; + private Parser parser; @JsonCreator @@ -63,7 +63,6 @@ public ProtobufInputRowParser( this.parseSpec = parseSpec; this.descriptorFilePath = descriptorFilePath; this.protoMessageType = protoMessageType; - this.parser = parseSpec.makeParser(); this.descriptor = getDescriptor(descriptorFilePath); } @@ -82,6 +81,11 @@ public ProtobufInputRowParser withParseSpec(ParseSpec parseSpec) @Override public InputRow parse(ByteBuffer input) { + if (parser == null) { + // parser should be created when it is really used to avoid unnecessary initialization of the underlying + // parseSpec. + parser = parseSpec.makeParser(); + } String json; try { DynamicMessage message = DynamicMessage.parseFrom(descriptor, ByteString.copyFrom(input)); diff --git a/extensions-core/protobuf-extensions/src/test/java/io/druid/data/input/protobuf/ProtobufInputRowParserTest.java b/extensions-core/protobuf-extensions/src/test/java/io/druid/data/input/protobuf/ProtobufInputRowParserTest.java index 830de9971ff0..ab3744e018ab 100644 --- a/extensions-core/protobuf-extensions/src/test/java/io/druid/data/input/protobuf/ProtobufInputRowParserTest.java +++ b/extensions-core/protobuf-extensions/src/test/java/io/druid/data/input/protobuf/ProtobufInputRowParserTest.java @@ -19,11 +19,13 @@ package io.druid.data.input.protobuf; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import io.druid.data.input.InputRow; import io.druid.data.input.impl.DimensionSchema; import io.druid.data.input.impl.DimensionsSpec; import io.druid.data.input.impl.JSONParseSpec; +import io.druid.data.input.impl.JavaScriptParseSpec; import io.druid.data.input.impl.ParseSpec; import io.druid.data.input.impl.StringDimensionSchema; import io.druid.data.input.impl.TimestampSpec; @@ -31,10 +33,14 @@ import io.druid.java.util.common.parsers.JSONPathFieldType; import io.druid.java.util.common.parsers.JSONPathSpec; import io.druid.java.util.common.parsers.ParseException; +import io.druid.js.JavaScriptConfig; +import org.hamcrest.CoreMatchers; import org.joda.time.DateTime; import org.joda.time.chrono.ISOChronology; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import java.io.ByteArrayOutputStream; import java.nio.ByteBuffer; @@ -44,6 +50,9 @@ public class ProtobufInputRowParserTest { + @Rule + public ExpectedException expectedException = ExpectedException.none(); + private ParseSpec parseSpec; @Before @@ -162,6 +171,32 @@ public void testParse() throws Exception assertEquals(816.0F, row.getFloatMetric("someLongColumn"), 0.0); } + @Test + public void testDisableJavaScript() + { + final JavaScriptParseSpec parseSpec = new JavaScriptParseSpec( + new TimestampSpec("timestamp", "auto", null), + new DimensionsSpec( + DimensionsSpec.getDefaultSchemas( + ImmutableList.of( + "dim1", + "dim2" + ) + ), + null, + null + ), + "func", + new JavaScriptConfig(false) + ); + final ProtobufInputRowParser parser = new ProtobufInputRowParser(parseSpec, "prototest.desc", "ProtoTestEvent"); + + expectedException.expect(CoreMatchers.instanceOf(IllegalStateException.class)); + expectedException.expectMessage("JavaScript is disabled"); + + parser.parse(ByteBuffer.allocate(1)); + } + private void assertDimensionEquals(InputRow row, String dimension, Object expected) { List values = row.getDimension(dimension); diff --git a/processing/src/main/java/io/druid/query/aggregation/JavaScriptAggregatorFactory.java b/processing/src/main/java/io/druid/query/aggregation/JavaScriptAggregatorFactory.java index 869ce24603ba..a558f2565048 100644 --- a/processing/src/main/java/io/druid/query/aggregation/JavaScriptAggregatorFactory.java +++ b/processing/src/main/java/io/druid/query/aggregation/JavaScriptAggregatorFactory.java @@ -75,7 +75,6 @@ public JavaScriptAggregatorFactory( Preconditions.checkNotNull(fnAggregate, "Must have a valid, non-null fnAggregate"); Preconditions.checkNotNull(fnReset, "Must have a valid, non-null fnReset"); Preconditions.checkNotNull(fnCombine, "Must have a valid, non-null fnCombine"); - Preconditions.checkState(config.isEnabled(), "JavaScript is disabled"); this.name = name; this.fieldNames = fieldNames; @@ -89,7 +88,7 @@ public JavaScriptAggregatorFactory( @Override public Aggregator factorize(final ColumnSelectorFactory columnFactory) { - checkCompiledScript(); + checkAndCompileScript(); return new JavaScriptAggregator( Lists.transform( fieldNames, @@ -109,7 +108,7 @@ public ObjectColumnSelector apply(@Nullable String s) @Override public BufferAggregator factorizeBuffered(final ColumnSelectorFactory columnSelectorFactory) { - checkCompiledScript(); + checkAndCompileScript(); return new JavaScriptBufferAggregator( Lists.transform( fieldNames, @@ -135,7 +134,7 @@ public Comparator getComparator() @Override public Object combine(Object lhs, Object rhs) { - checkCompiledScript(); + checkAndCompileScript(); return compiledScript.combine(((Number) lhs).doubleValue(), ((Number) rhs).doubleValue()); } @@ -155,7 +154,7 @@ public void reset(ColumnValueSelector selector) @Override public void fold(ColumnValueSelector selector) { - checkCompiledScript(); + checkAndCompileScript(); combined = compiledScript.combine(combined, selector.getDouble()); } @@ -299,9 +298,22 @@ public String toString() '}'; } - private void checkCompiledScript() + /** + * This class can be used by multiple threads, so this function should be thread-safe to avoid extra + * script compilation. + */ + private void checkAndCompileScript() { - compiledScript = compiledScript == null ? compileScript(fnAggregate, fnReset, fnCombine) : compiledScript; + if (compiledScript == null) { + synchronized (config) { + if (compiledScript == null) { + // JavaScript configuration should be checked when it's actually used because someone might still want Druid + // nodes to be able to deserialize JavaScript-based objects even though JavaScript is disabled. + Preconditions.checkState(config.isEnabled(), "JavaScript is disabled"); + compiledScript = compileScript(fnAggregate, fnReset, fnCombine); + } + } + } } @VisibleForTesting diff --git a/processing/src/main/java/io/druid/query/aggregation/post/JavaScriptPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/post/JavaScriptPostAggregator.java index 5aae34206207..4ed4d847db87 100644 --- a/processing/src/main/java/io/druid/query/aggregation/post/JavaScriptPostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/post/JavaScriptPostAggregator.java @@ -85,6 +85,7 @@ public double apply(Object[] args) private final String name; private final List fieldNames; private final String function; + private final JavaScriptConfig config; // This variable is lazily initialized to avoid unnecessary JavaScript compilation during JSON serde private Function fn; @@ -100,11 +101,11 @@ public JavaScriptPostAggregator( Preconditions.checkNotNull(name, "Must have a valid, non-null post-aggregator name"); Preconditions.checkNotNull(fieldNames, "Must have a valid, non-null fieldNames"); Preconditions.checkNotNull(function, "Must have a valid, non-null function"); - Preconditions.checkState(config.isEnabled(), "JavaScript is disabled"); this.name = name; this.fieldNames = fieldNames; this.function = function; + this.config = config; } @Override @@ -122,15 +123,36 @@ public Comparator getComparator() @Override public Object compute(Map combinedAggregators) { + checkAndCompileScript(); final Object[] args = new Object[fieldNames.size()]; int i = 0; for (String field : fieldNames) { args[i++] = combinedAggregators.get(field); } - fn = fn == null ? compile(function) : fn; return fn.apply(args); } + /** + * {@link #compute} can be called by multiple threads, so this function should be thread-safe to avoid extra + * script compilation. + */ + private void checkAndCompileScript() + { + if (fn == null) { + // Synchronizing here can degrade the performance significantly because this method is called per input row. + // However, early compilation of JavaScript functions can occur some memory issues due to unnecessary compilation + // involving Java class generation each time, and thus this will be better. + synchronized (config) { + if (fn == null) { + // JavaScript configuration should be checked when it's actually used because someone might still want Druid + // nodes to be able to deserialize JavaScript-based objects even though JavaScript is disabled. + Preconditions.checkState(config.isEnabled(), "JavaScript is disabled"); + fn = compile(function); + } + } + } + } + @Override public byte[] getCacheKey() { diff --git a/processing/src/main/java/io/druid/query/extraction/JavaScriptExtractionFn.java b/processing/src/main/java/io/druid/query/extraction/JavaScriptExtractionFn.java index c4483c11eb20..be7f6cd78098 100644 --- a/processing/src/main/java/io/druid/query/extraction/JavaScriptExtractionFn.java +++ b/processing/src/main/java/io/druid/query/extraction/JavaScriptExtractionFn.java @@ -67,6 +67,7 @@ public String apply(Object input) private final String function; private final boolean injective; + private final JavaScriptConfig config; // This variable is lazily initialized to avoid unnecessary JavaScript compilation during JSON serde private Function fn; @@ -79,10 +80,10 @@ public JavaScriptExtractionFn( ) { Preconditions.checkNotNull(function, "function must not be null"); - Preconditions.checkState(config.isEnabled(), "JavaScript is disabled"); this.function = function; this.injective = injective; + this.config = config; } @JsonProperty @@ -111,10 +112,28 @@ public byte[] getCacheKey() @Nullable public String apply(@Nullable Object value) { - fn = fn == null ? compile(function) : fn; + checkAndCompileScript(); return Strings.emptyToNull(fn.apply(value)); } + /** + * {@link #apply(Object)} can be called by multiple threads, so this function should be thread-safe to avoid extra + * script compilation. + */ + private void checkAndCompileScript() + { + if (fn == null) { + synchronized (config) { + if (fn == null) { + // JavaScript configuration should be checked when it's actually used because someone might still want Druid + // nodes to be able to deserialize JavaScript-based objects even though JavaScript is disabled. + Preconditions.checkState(config.isEnabled(), "JavaScript is disabled"); + fn = compile(function); + } + } + } + } + @Override @Nullable public String apply(@Nullable String value) diff --git a/processing/src/main/java/io/druid/query/filter/JavaScriptDimFilter.java b/processing/src/main/java/io/druid/query/filter/JavaScriptDimFilter.java index 6fb8749259ab..0c25b2a4ab19 100644 --- a/processing/src/main/java/io/druid/query/filter/JavaScriptDimFilter.java +++ b/processing/src/main/java/io/druid/query/filter/JavaScriptDimFilter.java @@ -40,6 +40,7 @@ public class JavaScriptDimFilter implements DimFilter private final String dimension; private final String function; private final ExtractionFn extractionFn; + private final JavaScriptConfig config; // This variable is lazily initialized to avoid unnecessary JavaScript compilation during JSON serde private JavaScriptPredicateFactory predicateFactory; @@ -54,10 +55,10 @@ public JavaScriptDimFilter( { Preconditions.checkArgument(dimension != null, "dimension must not be null"); Preconditions.checkArgument(function != null, "function must not be null"); - Preconditions.checkState(config.isEnabled(), "JavaScript is disabled"); this.dimension = dimension; this.function = function; this.extractionFn = extractionFn; + this.config = config; } @JsonProperty @@ -104,12 +105,28 @@ public DimFilter optimize() @Override public Filter toFilter() { - predicateFactory = predicateFactory == null ? - new JavaScriptPredicateFactory(function, extractionFn) : - predicateFactory; + checkAndCreatePredicateFactory(); return new JavaScriptFilter(dimension, predicateFactory); } + /** + * This class can be used by multiple threads, so this function should be thread-safe to avoid extra + * script compilation. + */ + private void checkAndCreatePredicateFactory() + { + if (predicateFactory == null) { + synchronized (config) { + if (predicateFactory == null) { + // JavaScript configuration should be checked when it's actually used because someone might still want Druid + // nodes to be able to deserialize JavaScript-based objects even though JavaScript is disabled. + Preconditions.checkState(config.isEnabled(), "JavaScript is disabled"); + predicateFactory = new JavaScriptPredicateFactory(function, extractionFn); + } + } + } + } + @Override public RangeSet getDimensionRangeSet(String dimension) { diff --git a/processing/src/test/java/io/druid/query/aggregation/JavaScriptAggregatorTest.java b/processing/src/test/java/io/druid/query/aggregation/JavaScriptAggregatorTest.java index 302cd0b835fd..ea46da64a2bc 100644 --- a/processing/src/test/java/io/druid/query/aggregation/JavaScriptAggregatorTest.java +++ b/processing/src/test/java/io/druid/query/aggregation/JavaScriptAggregatorTest.java @@ -280,9 +280,6 @@ public void testAggregateStrings() @Test public void testJavaScriptDisabledFactorize() { - expectedException.expect(IllegalStateException.class); - expectedException.expectMessage("JavaScript is disabled"); - final JavaScriptAggregatorFactory factory = new JavaScriptAggregatorFactory( "foo", ImmutableList.of("foo"), @@ -291,13 +288,16 @@ public void testJavaScriptDisabledFactorize() scriptDoubleSum.get("fnCombine"), new JavaScriptConfig(false) ); + + expectedException.expect(IllegalStateException.class); + expectedException.expectMessage("JavaScript is disabled"); + factory.factorize(DUMMY_COLUMN_SELECTOR_FACTORY); + Assert.assertTrue(false); } @Test public void testJavaScriptDisabledFactorizeBuffered() { - expectedException.expect(IllegalStateException.class); - expectedException.expectMessage("JavaScript is disabled"); final JavaScriptAggregatorFactory factory = new JavaScriptAggregatorFactory( "foo", ImmutableList.of("foo"), @@ -306,6 +306,11 @@ public void testJavaScriptDisabledFactorizeBuffered() scriptDoubleSum.get("fnCombine"), new JavaScriptConfig(false) ); + + expectedException.expect(IllegalStateException.class); + expectedException.expectMessage("JavaScript is disabled"); + factory.factorizeBuffered(DUMMY_COLUMN_SELECTOR_FACTORY); + Assert.assertTrue(false); } public static void main(String... args) throws Exception diff --git a/processing/src/test/java/io/druid/query/aggregation/post/JavaScriptPostAggregatorTest.java b/processing/src/test/java/io/druid/query/aggregation/post/JavaScriptPostAggregatorTest.java index 0c29cfd21055..22013588a404 100644 --- a/processing/src/test/java/io/druid/query/aggregation/post/JavaScriptPostAggregatorTest.java +++ b/processing/src/test/java/io/druid/query/aggregation/post/JavaScriptPostAggregatorTest.java @@ -27,6 +27,7 @@ import org.junit.Test; import org.junit.rules.ExpectedException; +import java.util.HashMap; import java.util.Map; public class JavaScriptPostAggregatorTest @@ -59,13 +60,15 @@ public void testCompute() public void testComputeJavaScriptNotAllowed() { String absPercentFunction = "function(delta, total) { return 100 * Math.abs(delta) / total; }"; - expectedException.expect(IllegalStateException.class); - expectedException.expectMessage("JavaScript is disabled"); - new JavaScriptPostAggregator( + JavaScriptPostAggregator aggregator = new JavaScriptPostAggregator( "absPercent", Lists.newArrayList("delta", "total"), absPercentFunction, new JavaScriptConfig(false) ); + + expectedException.expect(IllegalStateException.class); + expectedException.expectMessage("JavaScript is disabled"); + aggregator.compute(new HashMap<>()); } } diff --git a/processing/src/test/java/io/druid/query/extraction/JavaScriptExtractionFnTest.java b/processing/src/test/java/io/druid/query/extraction/JavaScriptExtractionFnTest.java index 929e649c2750..dc4494dadbde 100644 --- a/processing/src/test/java/io/druid/query/extraction/JavaScriptExtractionFnTest.java +++ b/processing/src/test/java/io/druid/query/extraction/JavaScriptExtractionFnTest.java @@ -64,10 +64,13 @@ public void testJavascriptSubstring() @Test public void testJavascriptNotAllowed() { - expectedException.expect(IllegalStateException.class); - expectedException.expectMessage("JavaScript is disabled"); String function = "function(str) { return str.substring(0,3); }"; ExtractionFn extractionFn = new JavaScriptExtractionFn(function, false, new JavaScriptConfig(false)); + + expectedException.expect(IllegalStateException.class); + expectedException.expectMessage("JavaScript is disabled"); + extractionFn.apply("hey"); + Assert.assertTrue(false); } @Test diff --git a/processing/src/test/java/io/druid/query/filter/JavaScriptDimFilterTest.java b/processing/src/test/java/io/druid/query/filter/JavaScriptDimFilterTest.java index 659554bc9b70..5c43f2390282 100644 --- a/processing/src/test/java/io/druid/query/filter/JavaScriptDimFilterTest.java +++ b/processing/src/test/java/io/druid/query/filter/JavaScriptDimFilterTest.java @@ -98,9 +98,11 @@ public void testToFilter() @Test public void testToFilterNotAllowed() { - expectedException.expect(IllegalStateException.class); - expectedException.expectMessage("JavaScript is disabled"); JavaScriptDimFilter javaScriptDimFilter = new JavaScriptDimFilter("dim", FN1, null, new JavaScriptConfig(false)); + expectedException.expect(IllegalStateException.class); + expectedException.expectMessage("JavaScript is disabled"); + javaScriptDimFilter.toFilter(); + Assert.assertTrue(false); } } diff --git a/server/src/main/java/io/druid/segment/indexing/DataSchema.java b/server/src/main/java/io/druid/segment/indexing/DataSchema.java index eb5f88a52dc2..bc4c196612d6 100644 --- a/server/src/main/java/io/druid/segment/indexing/DataSchema.java +++ b/server/src/main/java/io/druid/segment/indexing/DataSchema.java @@ -53,6 +53,8 @@ public class DataSchema private final ObjectMapper jsonMapper; + private InputRowParser cachedParser; + @JsonCreator public DataSchema( @JsonProperty("dataSource") String dataSource, @@ -108,6 +110,10 @@ public InputRowParser getParser() return null; } + if (cachedParser != null) { + return cachedParser; + } + final InputRowParser inputRowParser = jsonMapper.convertValue(this.parser, InputRowParser.class); final Set dimensionExclusions = Sets.newHashSet(); @@ -141,7 +147,7 @@ public InputRowParser getParser() ); } - return inputRowParser.withParseSpec( + cachedParser = inputRowParser.withParseSpec( inputRowParser.getParseSpec() .withDimensionsSpec( dimensionsSpec @@ -151,12 +157,14 @@ public InputRowParser getParser() ) ); } else { - return inputRowParser; + cachedParser = inputRowParser; } } else { log.warn("No parseSpec in parser has been specified."); - return inputRowParser; + cachedParser = inputRowParser; } + + return cachedParser; } @JsonProperty("metricsSpec") From be6f1056c6bf57c84fb6707cc610eec07dcbd880 Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Sat, 7 Oct 2017 20:10:08 +0900 Subject: [PATCH 4/7] Fix test fail --- .../data/input/impl/StringInputRowParser.java | 3 ++- .../segment/indexing/DataSchemaTest.java | 20 +++++++++++++------ 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/api/src/main/java/io/druid/data/input/impl/StringInputRowParser.java b/api/src/main/java/io/druid/data/input/impl/StringInputRowParser.java index ec673271f0a6..fdf3f5d031a6 100644 --- a/api/src/main/java/io/druid/data/input/impl/StringInputRowParser.java +++ b/api/src/main/java/io/druid/data/input/impl/StringInputRowParser.java @@ -22,6 +22,7 @@ import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; import com.google.common.base.Charsets; +import com.google.common.base.Preconditions; import io.druid.data.input.ByteBufferInputRowParser; import io.druid.data.input.InputRow; import io.druid.java.util.common.parsers.ParseException; @@ -54,7 +55,7 @@ public StringInputRowParser( @JsonProperty("encoding") String encoding ) { - this.parseSpec = parseSpec; + this.parseSpec = Preconditions.checkNotNull(parseSpec, "parseSpec"); this.mapParser = new MapInputRowParser(parseSpec); if (encoding != null) { diff --git a/server/src/test/java/io/druid/segment/indexing/DataSchemaTest.java b/server/src/test/java/io/druid/segment/indexing/DataSchemaTest.java index d415875ad737..08b3523efd82 100644 --- a/server/src/test/java/io/druid/segment/indexing/DataSchemaTest.java +++ b/server/src/test/java/io/druid/segment/indexing/DataSchemaTest.java @@ -19,6 +19,7 @@ package io.druid.segment.indexing; +import com.fasterxml.jackson.databind.JsonMappingException; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -35,14 +36,20 @@ import io.druid.query.aggregation.DoubleSumAggregatorFactory; import io.druid.segment.TestHelper; import io.druid.segment.indexing.granularity.ArbitraryGranularitySpec; +import org.hamcrest.CoreMatchers; import org.junit.Assert; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import java.util.Arrays; import java.util.Map; public class DataSchemaTest { + @Rule + public ExpectedException expectedException = ExpectedException.none(); + private final ObjectMapper jsonMapper = TestHelper.getJsonMapper(); @Test @@ -200,13 +207,14 @@ public void testSerdeWithInvalidParserMap() throws Exception DataSchema.class ); - try { - schema.getParser(); - Assert.fail("should've failed to get parser."); - } - catch (IllegalArgumentException ex) { + expectedException.expect(CoreMatchers.instanceOf(IllegalArgumentException.class)); + expectedException.expectCause(CoreMatchers.instanceOf(JsonMappingException.class)); + expectedException.expectMessage( + "Instantiation of [simple type, class io.druid.data.input.impl.StringInputRowParser] value failed: parseSpec" + ); - } + // Jackson creates a default type parser (StringInputRowParser) for an invalid type. + schema.getParser(); } @Test From 135c8af11bc9318c9a89781fcd84a9cd20d42450 Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Sat, 7 Oct 2017 22:37:22 +0900 Subject: [PATCH 5/7] Fix test --- .../druid/data/input/impl/StringInputRowParser.java | 13 +++++++------ .../io/druid/cli/validate/DruidJsonValidator.java | 1 + 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/api/src/main/java/io/druid/data/input/impl/StringInputRowParser.java b/api/src/main/java/io/druid/data/input/impl/StringInputRowParser.java index fdf3f5d031a6..74467ccbf483 100644 --- a/api/src/main/java/io/druid/data/input/impl/StringInputRowParser.java +++ b/api/src/main/java/io/druid/data/input/impl/StringInputRowParser.java @@ -124,13 +124,18 @@ private Map buildStringKeyMap(ByteBuffer input) return theMap; } - public void startFileFromBeginning() + public void initializeParser() { if (parser == null) { // parser should be created when it is really used to avoid unnecessary initialization of the underlying // parseSpec. parser = parseSpec.makeParser(); } + } + + public void startFileFromBeginning() + { + initializeParser(); parser.startFileFromBeginning(); } @@ -143,11 +148,7 @@ public InputRow parse(@Nullable String input) @Nullable private Map parseString(@Nullable String inputString) { - if (parser == null) { - // parser should be created when it is really used to avoid unnecessary initialization of the underlying - // parseSpec. - parser = parseSpec.makeParser(); - } + initializeParser(); return parser.parse(inputString); } diff --git a/services/src/main/java/io/druid/cli/validate/DruidJsonValidator.java b/services/src/main/java/io/druid/cli/validate/DruidJsonValidator.java index 5a4efb721a66..8846f69aea5a 100644 --- a/services/src/main/java/io/druid/cli/validate/DruidJsonValidator.java +++ b/services/src/main/java/io/druid/cli/validate/DruidJsonValidator.java @@ -179,6 +179,7 @@ public void write(char[] cbuf, int off, int len) logWriter.write("cannot find proper spec from 'file'.. regarding it as a json spec"); parser = jsonMapper.readValue(jsonFile, StringInputRowParser.class); } + parser.initializeParser(); if (resource != null) { final CharSource source; if (new File(resource).isFile()) { From 570522249aa4f3ca52fb3b105ba61651255547f0 Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Sun, 8 Oct 2017 00:26:02 +0900 Subject: [PATCH 6/7] Fix KafkaIndexTaskTest --- .../io/druid/indexing/kafka/KafkaIndexTaskTest.java | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/extensions-core/kafka-indexing-service/src/test/java/io/druid/indexing/kafka/KafkaIndexTaskTest.java b/extensions-core/kafka-indexing-service/src/test/java/io/druid/indexing/kafka/KafkaIndexTaskTest.java index 3f7ba71ba1e7..d5c841ebd6c8 100644 --- a/extensions-core/kafka-indexing-service/src/test/java/io/druid/indexing/kafka/KafkaIndexTaskTest.java +++ b/extensions-core/kafka-indexing-service/src/test/java/io/druid/indexing/kafka/KafkaIndexTaskTest.java @@ -1483,7 +1483,7 @@ private KafkaIndexTask createTask( final KafkaIndexTask task = new KafkaIndexTask( taskId, null, - DATA_SCHEMA, + cloneDataSchema(), tuningConfig, ioConfig, null, @@ -1494,6 +1494,17 @@ private KafkaIndexTask createTask( return task; } + private static DataSchema cloneDataSchema() + { + return new DataSchema( + DATA_SCHEMA.getDataSource(), + DATA_SCHEMA.getParserMap(), + DATA_SCHEMA.getAggregators(), + DATA_SCHEMA.getGranularitySpec(), + objectMapper + ); + } + private QueryRunnerFactoryConglomerate makeTimeseriesOnlyConglomerate() { IntervalChunkingQueryRunnerDecorator queryRunnerDecorator = new IntervalChunkingQueryRunnerDecorator( From f002c3caf2ce1d9bebf7c4c673c18a7f500a7a92 Mon Sep 17 00:00:00 2001 From: Jihoon Son Date: Sun, 8 Oct 2017 10:37:50 +0900 Subject: [PATCH 7/7] Move config check --- .../query/aggregation/JavaScriptAggregatorFactory.java | 7 ++++--- .../query/aggregation/post/JavaScriptPostAggregator.java | 7 ++++--- .../io/druid/query/extraction/JavaScriptExtractionFn.java | 7 ++++--- .../java/io/druid/query/filter/JavaScriptDimFilter.java | 7 ++++--- 4 files changed, 16 insertions(+), 12 deletions(-) diff --git a/processing/src/main/java/io/druid/query/aggregation/JavaScriptAggregatorFactory.java b/processing/src/main/java/io/druid/query/aggregation/JavaScriptAggregatorFactory.java index a558f2565048..70cccb25ce5f 100644 --- a/processing/src/main/java/io/druid/query/aggregation/JavaScriptAggregatorFactory.java +++ b/processing/src/main/java/io/druid/query/aggregation/JavaScriptAggregatorFactory.java @@ -305,11 +305,12 @@ public String toString() private void checkAndCompileScript() { if (compiledScript == null) { + // JavaScript configuration should be checked when it's actually used because someone might still want Druid + // nodes to be able to deserialize JavaScript-based objects even though JavaScript is disabled. + Preconditions.checkState(config.isEnabled(), "JavaScript is disabled"); + synchronized (config) { if (compiledScript == null) { - // JavaScript configuration should be checked when it's actually used because someone might still want Druid - // nodes to be able to deserialize JavaScript-based objects even though JavaScript is disabled. - Preconditions.checkState(config.isEnabled(), "JavaScript is disabled"); compiledScript = compileScript(fnAggregate, fnReset, fnCombine); } } diff --git a/processing/src/main/java/io/druid/query/aggregation/post/JavaScriptPostAggregator.java b/processing/src/main/java/io/druid/query/aggregation/post/JavaScriptPostAggregator.java index 4ed4d847db87..78aaec95f08b 100644 --- a/processing/src/main/java/io/druid/query/aggregation/post/JavaScriptPostAggregator.java +++ b/processing/src/main/java/io/druid/query/aggregation/post/JavaScriptPostAggregator.java @@ -139,14 +139,15 @@ public Object compute(Map combinedAggregators) private void checkAndCompileScript() { if (fn == null) { + // JavaScript configuration should be checked when it's actually used because someone might still want Druid + // nodes to be able to deserialize JavaScript-based objects even though JavaScript is disabled. + Preconditions.checkState(config.isEnabled(), "JavaScript is disabled"); + // Synchronizing here can degrade the performance significantly because this method is called per input row. // However, early compilation of JavaScript functions can occur some memory issues due to unnecessary compilation // involving Java class generation each time, and thus this will be better. synchronized (config) { if (fn == null) { - // JavaScript configuration should be checked when it's actually used because someone might still want Druid - // nodes to be able to deserialize JavaScript-based objects even though JavaScript is disabled. - Preconditions.checkState(config.isEnabled(), "JavaScript is disabled"); fn = compile(function); } } diff --git a/processing/src/main/java/io/druid/query/extraction/JavaScriptExtractionFn.java b/processing/src/main/java/io/druid/query/extraction/JavaScriptExtractionFn.java index be7f6cd78098..1d4e479acd8d 100644 --- a/processing/src/main/java/io/druid/query/extraction/JavaScriptExtractionFn.java +++ b/processing/src/main/java/io/druid/query/extraction/JavaScriptExtractionFn.java @@ -123,11 +123,12 @@ public String apply(@Nullable Object value) private void checkAndCompileScript() { if (fn == null) { + // JavaScript configuration should be checked when it's actually used because someone might still want Druid + // nodes to be able to deserialize JavaScript-based objects even though JavaScript is disabled. + Preconditions.checkState(config.isEnabled(), "JavaScript is disabled"); + synchronized (config) { if (fn == null) { - // JavaScript configuration should be checked when it's actually used because someone might still want Druid - // nodes to be able to deserialize JavaScript-based objects even though JavaScript is disabled. - Preconditions.checkState(config.isEnabled(), "JavaScript is disabled"); fn = compile(function); } } diff --git a/processing/src/main/java/io/druid/query/filter/JavaScriptDimFilter.java b/processing/src/main/java/io/druid/query/filter/JavaScriptDimFilter.java index 0c25b2a4ab19..7c6bdab7325b 100644 --- a/processing/src/main/java/io/druid/query/filter/JavaScriptDimFilter.java +++ b/processing/src/main/java/io/druid/query/filter/JavaScriptDimFilter.java @@ -116,11 +116,12 @@ public Filter toFilter() private void checkAndCreatePredicateFactory() { if (predicateFactory == null) { + // JavaScript configuration should be checked when it's actually used because someone might still want Druid + // nodes to be able to deserialize JavaScript-based objects even though JavaScript is disabled. + Preconditions.checkState(config.isEnabled(), "JavaScript is disabled"); + synchronized (config) { if (predicateFactory == null) { - // JavaScript configuration should be checked when it's actually used because someone might still want Druid - // nodes to be able to deserialize JavaScript-based objects even though JavaScript is disabled. - Preconditions.checkState(config.isEnabled(), "JavaScript is disabled"); predicateFactory = new JavaScriptPredicateFactory(function, extractionFn); } }