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..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 @@ -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, @@ -64,11 +67,11 @@ public void verify(List usedCols) @Override public Parser makeParser() { - if (!config.isEnabled()) { - throw new ISE("JavaScript is disabled"); - } - - return new JavaScriptParser(function); + // 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; } @Override 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..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 @@ -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; @@ -43,10 +44,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( @@ -54,9 +55,8 @@ public StringInputRowParser( @JsonProperty("encoding") String encoding ) { - this.parseSpec = parseSpec; + this.parseSpec = Preconditions.checkNotNull(parseSpec, "parseSpec"); this.mapParser = new MapInputRowParser(parseSpec); - this.parser = parseSpec.makeParser(); if (encoding != null) { this.charset = Charset.forName(encoding); @@ -124,8 +124,18 @@ private Map buildStringKeyMap(ByteBuffer input) return theMap; } + 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(); } @@ -138,6 +148,7 @@ public InputRow parse(@Nullable String input) @Nullable private Map parseString(@Nullable String inputString) { + initializeParser(); return parser.parse(inputString); } 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/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( 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/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..70cccb25ce5f 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( @@ -83,17 +83,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) { + checkAndCompileScript(); return new JavaScriptAggregator( Lists.transform( fieldNames, @@ -106,13 +101,14 @@ public ObjectColumnSelector apply(@Nullable String s) } } ), - getCompiledScript() + compiledScript ); } @Override public BufferAggregator factorizeBuffered(final ColumnSelectorFactory columnSelectorFactory) { + checkAndCompileScript(); return new JavaScriptBufferAggregator( Lists.transform( fieldNames, @@ -125,7 +121,7 @@ public ObjectColumnSelector apply(@Nullable String s) } } ), - getCompiledScript() + compiledScript ); } @@ -138,7 +134,8 @@ public Comparator getComparator() @Override public Object combine(Object lhs, Object rhs) { - return getCompiledScript().combine(((Number) lhs).doubleValue(), ((Number) rhs).doubleValue()); + checkAndCompileScript(); + return compiledScript.combine(((Number) lhs).doubleValue(), ((Number) rhs).doubleValue()); } @Override @@ -157,7 +154,8 @@ public void reset(ColumnValueSelector selector) @Override public void fold(ColumnValueSelector selector) { - combined = getCompiledScript().combine(combined, selector.getDouble()); + checkAndCompileScript(); + combined = compiledScript.combine(combined, selector.getDouble()); } @Override @@ -300,13 +298,23 @@ public String toString() '}'; } - private JavaScriptAggregator.ScriptAggregator getCompiledScript() + /** + * This class can be used by multiple threads, so this function should be thread-safe to avoid extra + * script compilation. + */ + private void checkAndCompileScript() { if (compiledScript == null) { - throw new ISE("JavaScript is disabled"); - } + // 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"); - return compiledScript; + synchronized (config) { + if (compiledScript == null) { + 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 0b285c3e8c02..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 @@ -85,8 +85,10 @@ public double apply(Object[] args) private final String name; private final List fieldNames; private final String function; + private final JavaScriptConfig config; - 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 +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.fn = compile(function); + this.config = config; } @Override @@ -122,6 +123,7 @@ public Comparator getComparator() @Override public Object compute(Map combinedAggregators) { + checkAndCompileScript(); final Object[] args = new Object[fieldNames.size()]; int i = 0; for (String field : fieldNames) { @@ -130,6 +132,28 @@ public Object compute(Map combinedAggregators) 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) { + // 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) { + fn = compile(function); + } + } + } + } + @Override public byte[] getCacheKey() { @@ -179,9 +203,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 +219,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..1d4e479acd8d 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,8 +66,11 @@ public String apply(Object input) } private final String function; - private final Function fn; private final boolean injective; + private final JavaScriptConfig config; + + // This variable is lazily initialized to avoid unnecessary JavaScript compilation during JSON serde + private Function fn; @JsonCreator public JavaScriptExtractionFn( @@ -81,12 +83,7 @@ public JavaScriptExtractionFn( this.function = function; this.injective = injective; - - if (config.isEnabled()) { - this.fn = compile(function); - } else { - this.fn = null; - } + this.config = config; } @JsonProperty @@ -114,12 +111,28 @@ public byte[] getCacheKey() @Override @Nullable public String apply(@Nullable Object value) + { + 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) { - throw new ISE("JavaScript is disabled"); - } + // 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"); - return Strings.emptyToNull(fn.apply(value)); + synchronized (config) { + if (fn == null) { + fn = compile(function); + } + } + } } @Override 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..7c6bdab7325b 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; @@ -43,7 +42,8 @@ public class JavaScriptDimFilter implements DimFilter 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( @@ -59,12 +59,6 @@ public JavaScriptDimFilter( this.function = function; this.extractionFn = extractionFn; this.config = config; - - if (config.isEnabled()) { - this.predicateFactory = new JavaScriptPredicateFactory(function, extractionFn); - } else { - this.predicateFactory = null; - } } @JsonProperty @@ -111,13 +105,29 @@ public DimFilter optimize() @Override public Filter toFilter() { - if (!config.isEnabled()) { - throw new ISE("JavaScript is disabled"); - } - + 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) { + // 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) { + predicateFactory = new JavaScriptPredicateFactory(function, extractionFn); + } + } + } + } + @Override public RangeSet getDimensionRangeSet(String dimension) { 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/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") 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)); } 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 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()) {