From e36b84b7950c5b67f000c63ef43daa4713ce655c Mon Sep 17 00:00:00 2001 From: kamaci Date: Mon, 26 Nov 2018 14:26:37 +0300 Subject: [PATCH 1/6] Double-checked locking bug is fixed. --- aws-common/pom.xml | 5 ++ .../LazyFileSessionCredentialsProvider.java | 26 ++++++++--- pom.xml | 1 + processing/pom.xml | 5 ++ .../JavaScriptAggregatorFactory.java | 46 +++++++++++++------ .../post/JavaScriptPostAggregator.java | 34 +++++++++----- .../extraction/JavaScriptExtractionFn.java | 28 +++++++---- .../query/filter/JavaScriptDimFilter.java | 29 ++++++++---- 8 files changed, 125 insertions(+), 49 deletions(-) diff --git a/aws-common/pom.xml b/aws-common/pom.xml index 4b941c46c7d8..941dc2847b01 100644 --- a/aws-common/pom.xml +++ b/aws-common/pom.xml @@ -45,6 +45,11 @@ com.amazonaws aws-java-sdk-s3 + + org.checkerframework + checker + ${checkerframework.version} + diff --git a/aws-common/src/main/java/org/apache/druid/common/aws/LazyFileSessionCredentialsProvider.java b/aws-common/src/main/java/org/apache/druid/common/aws/LazyFileSessionCredentialsProvider.java index 7fc046b3165a..450e77435922 100644 --- a/aws-common/src/main/java/org/apache/druid/common/aws/LazyFileSessionCredentialsProvider.java +++ b/aws-common/src/main/java/org/apache/druid/common/aws/LazyFileSessionCredentialsProvider.java @@ -21,27 +21,41 @@ import com.amazonaws.auth.AWSCredentials; import com.amazonaws.auth.AWSCredentialsProvider; +import org.checkerframework.checker.nullness.qual.EnsuresNonNull; + +import javax.annotation.Nullable; public class LazyFileSessionCredentialsProvider implements AWSCredentialsProvider { - private AWSCredentialsConfig config; - private FileSessionCredentialsProvider provider; + private final AWSCredentialsConfig config; + + /** + * The field is declared volatile in order to ensure safe publication of the object + * in {@link #getUnderlyingProvider()} without worrying about final modifiers + * on the fields of the created object + */ + @Nullable + private volatile FileSessionCredentialsProvider provider; public LazyFileSessionCredentialsProvider(AWSCredentialsConfig config) { this.config = config; } + @EnsuresNonNull("provider") private FileSessionCredentialsProvider getUnderlyingProvider() { - if (provider == null) { + FileSessionCredentialsProvider syncedProvider = provider; + if (syncedProvider == null) { synchronized (config) { - if (provider == null) { - provider = new FileSessionCredentialsProvider(config.getFileSessionCredentials()); + syncedProvider = provider; + if (syncedProvider == null) { + syncedProvider = new FileSessionCredentialsProvider(config.getFileSessionCredentials()); + provider = syncedProvider; } } } - return provider; + return syncedProvider; } @Override diff --git a/pom.xml b/pom.xml index 4c2938ebad21..46a1355d7287 100644 --- a/pom.xml +++ b/pom.xml @@ -100,6 +100,7 @@ 2.5.5 3.4.11 + 2.5.7 apache.snapshots Apache Snapshot Repository https://repository.apache.org/snapshots diff --git a/processing/pom.xml b/processing/pom.xml index 4963e32acbe8..c4d817ca9262 100644 --- a/processing/pom.xml +++ b/processing/pom.xml @@ -111,6 +111,11 @@ org.ow2.asm asm-commons + + org.checkerframework + checker + ${checkerframework.version} + diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java index e747ad68d73d..acfb164258e1 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java @@ -33,12 +33,15 @@ import org.apache.druid.segment.BaseObjectColumnValueSelector; import org.apache.druid.segment.ColumnSelectorFactory; import org.apache.druid.segment.ColumnValueSelector; +import org.checkerframework.checker.nullness.qual.EnsuresNonNull; +import org.checkerframework.checker.nullness.qual.MonotonicNonNull; import org.mozilla.javascript.Context; import org.mozilla.javascript.ContextAction; import org.mozilla.javascript.ContextFactory; import org.mozilla.javascript.Function; import org.mozilla.javascript.ScriptableObject; +import javax.annotation.Nullable; import java.lang.reflect.Array; import java.nio.ByteBuffer; import java.security.MessageDigest; @@ -49,6 +52,8 @@ import java.util.Objects; import java.util.stream.Collectors; +import static org.apache.druid.query.aggregation.JavaScriptAggregator.ScriptAggregator; + public class JavaScriptAggregatorFactory extends AggregatorFactory { private final String name; @@ -58,8 +63,14 @@ public class JavaScriptAggregatorFactory extends AggregatorFactory private final String fnCombine; private final JavaScriptConfig config; - // This variable is lazily initialized to avoid unnecessary JavaScript compilation during JSON serde - private JavaScriptAggregator.ScriptAggregator compiledScript; + /** + * The field is declared volatile in order to ensure safe publication of the object + * in {@link #compileScript(String, String, String)} without worrying about final modifiers + * on the fields of the created object + */ + @MonotonicNonNull + @Nullable + private volatile ScriptAggregator compiledScript; @JsonCreator public JavaScriptAggregatorFactory( @@ -89,7 +100,7 @@ public JavaScriptAggregatorFactory( @Override public Aggregator factorize(final ColumnSelectorFactory columnFactory) { - checkAndCompileScript(); + getCompiledScript(); return new JavaScriptAggregator( fieldNames.stream().map(columnFactory::makeColumnValueSelector).collect(Collectors.toList()), compiledScript @@ -99,7 +110,7 @@ public Aggregator factorize(final ColumnSelectorFactory columnFactory) @Override public BufferAggregator factorizeBuffered(final ColumnSelectorFactory columnSelectorFactory) { - checkAndCompileScript(); + getCompiledScript(); return new JavaScriptBufferAggregator( fieldNames.stream().map(columnSelectorFactory::makeColumnValueSelector).collect(Collectors.toList()), compiledScript @@ -115,7 +126,7 @@ public Comparator getComparator() @Override public Object combine(Object lhs, Object rhs) { - checkAndCompileScript(); + getCompiledScript(); return compiledScript.combine(((Number) lhs).doubleValue(), ((Number) rhs).doubleValue()); } @@ -135,7 +146,7 @@ public void reset(ColumnValueSelector selector) @Override public void fold(ColumnValueSelector selector) { - checkAndCompileScript(); + getCompiledScript(); combined = compiledScript.combine(combined, selector.getDouble()); } @@ -283,23 +294,28 @@ public String toString() * This class can be used by multiple threads, so this function should be thread-safe to avoid extra * script compilation. */ - private void checkAndCompileScript() + @EnsuresNonNull("compiledScript") + private ScriptAggregator getCompiledScript() { - 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"); + // 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"); + ScriptAggregator syncedCompiledScript = compiledScript; + if (syncedCompiledScript == null) { synchronized (config) { - if (compiledScript == null) { - compiledScript = compileScript(fnAggregate, fnReset, fnCombine); + syncedCompiledScript = compiledScript; + if (syncedCompiledScript == null) { + syncedCompiledScript = compileScript(fnAggregate, fnReset, fnCombine); + compiledScript = syncedCompiledScript; } } } + return syncedCompiledScript; } @VisibleForTesting - static JavaScriptAggregator.ScriptAggregator compileScript( + static ScriptAggregator compileScript( final String aggregate, final String reset, final String combine @@ -316,7 +332,7 @@ static JavaScriptAggregator.ScriptAggregator compileScript( final Function fnCombine = context.compileFunction(scope, combine, "combine", 1, null); Context.exit(); - return new JavaScriptAggregator.ScriptAggregator() + return new ScriptAggregator() { @Override public double aggregate(final double current, final BaseObjectColumnValueSelector[] selectorList) diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/post/JavaScriptPostAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/post/JavaScriptPostAggregator.java index 9bd6dc66a450..3d5d3860b057 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/post/JavaScriptPostAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/post/JavaScriptPostAggregator.java @@ -28,10 +28,13 @@ import org.apache.druid.query.aggregation.AggregatorFactory; import org.apache.druid.query.aggregation.PostAggregator; import org.apache.druid.query.cache.CacheKeyBuilder; +import org.checkerframework.checker.nullness.qual.EnsuresNonNull; +import org.checkerframework.checker.nullness.qual.MonotonicNonNull; import org.mozilla.javascript.Context; import org.mozilla.javascript.ContextFactory; import org.mozilla.javascript.ScriptableObject; +import javax.annotation.Nullable; import java.util.Comparator; import java.util.List; import java.util.Map; @@ -87,8 +90,14 @@ public double apply(Object[] args) private final String function; private final JavaScriptConfig config; - // This variable is lazily initialized to avoid unnecessary JavaScript compilation during JSON serde - private Function fn; + /** + * The field is declared volatile in order to ensure safe publication of the object + * in {@link #compile(String)} without worrying about final modifiers + * on the fields of the created object + */ + @MonotonicNonNull + @Nullable + private volatile Function fn; @JsonCreator public JavaScriptPostAggregator( @@ -136,19 +145,20 @@ public Object compute(Map combinedAggregators) * {@link #compute} can be called by multiple threads, so this function should be thread-safe to avoid extra * script compilation. */ + @EnsuresNonNull("fn") 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. + // 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"); + + Function syncedFn = fn; + if (syncedFn == null) { synchronized (config) { - if (fn == null) { - fn = compile(function); + syncedFn = fn; + if (syncedFn == null) { + syncedFn = compile(function); + fn = syncedFn; } } } diff --git a/processing/src/main/java/org/apache/druid/query/extraction/JavaScriptExtractionFn.java b/processing/src/main/java/org/apache/druid/query/extraction/JavaScriptExtractionFn.java index 06892ecd7724..8dba7a29ccb8 100644 --- a/processing/src/main/java/org/apache/druid/query/extraction/JavaScriptExtractionFn.java +++ b/processing/src/main/java/org/apache/druid/query/extraction/JavaScriptExtractionFn.java @@ -27,6 +27,8 @@ import org.apache.druid.common.config.NullHandling; import org.apache.druid.java.util.common.StringUtils; import org.apache.druid.js.JavaScriptConfig; +import org.checkerframework.checker.nullness.qual.EnsuresNonNull; +import org.checkerframework.checker.nullness.qual.MonotonicNonNull; import org.mozilla.javascript.Context; import org.mozilla.javascript.ContextFactory; import org.mozilla.javascript.ScriptableObject; @@ -69,8 +71,14 @@ public String apply(Object input) private final boolean injective; private final JavaScriptConfig config; - // This variable is lazily initialized to avoid unnecessary JavaScript compilation during JSON serde - private Function fn; + /** + * The field is declared volatile in order to ensure safe publication of the object + * in {@link #compile(String)} without worrying about final modifiers + * on the fields of the created object + */ + @MonotonicNonNull + @Nullable + private volatile Function fn; @JsonCreator public JavaScriptExtractionFn( @@ -120,16 +128,20 @@ public String apply(@Nullable Object value) * {@link #apply(Object)} can be called by multiple threads, so this function should be thread-safe to avoid extra * script compilation. */ + @EnsuresNonNull("fn") 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"); + // 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"); + Function syncedFn = fn; + if (syncedFn == null) { synchronized (config) { - if (fn == null) { - fn = compile(function); + syncedFn = fn; + if (syncedFn == null) { + syncedFn = compile(function); + fn = syncedFn; } } } diff --git a/processing/src/main/java/org/apache/druid/query/filter/JavaScriptDimFilter.java b/processing/src/main/java/org/apache/druid/query/filter/JavaScriptDimFilter.java index 0569aa481d6c..6fa06fa1d319 100644 --- a/processing/src/main/java/org/apache/druid/query/filter/JavaScriptDimFilter.java +++ b/processing/src/main/java/org/apache/druid/query/filter/JavaScriptDimFilter.java @@ -30,10 +30,13 @@ import org.apache.druid.js.JavaScriptConfig; import org.apache.druid.query.extraction.ExtractionFn; import org.apache.druid.segment.filter.JavaScriptFilter; +import org.checkerframework.checker.nullness.qual.EnsuresNonNull; +import org.checkerframework.checker.nullness.qual.MonotonicNonNull; import org.mozilla.javascript.Context; import org.mozilla.javascript.Function; import org.mozilla.javascript.ScriptableObject; +import javax.annotation.Nullable; import java.nio.ByteBuffer; import java.util.HashSet; @@ -44,8 +47,14 @@ public class JavaScriptDimFilter implements DimFilter private final ExtractionFn extractionFn; private final JavaScriptConfig config; - // This variable is lazily initialized to avoid unnecessary JavaScript compilation during JSON serde - private JavaScriptPredicateFactory predicateFactory; + /** + * The field is declared volatile in order to ensure safe publication of the object + * in {@link JavaScriptPredicateFactory(String, ExtractionFn)} without worrying about final modifiers + * on the fields of the created object + */ + @MonotonicNonNull + @Nullable + private volatile JavaScriptPredicateFactory predicateFactory; @JsonCreator public JavaScriptDimFilter( @@ -115,16 +124,20 @@ public Filter toFilter() * This class can be used by multiple threads, so this function should be thread-safe to avoid extra * script compilation. */ + @EnsuresNonNull("predicateFactory") 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"); + // 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"); + JavaScriptPredicateFactory syncedFnPredicateFactory = predicateFactory; + if (syncedFnPredicateFactory == null) { synchronized (config) { - if (predicateFactory == null) { - predicateFactory = new JavaScriptPredicateFactory(function, extractionFn); + syncedFnPredicateFactory = predicateFactory; + if (syncedFnPredicateFactory == null) { + syncedFnPredicateFactory = new JavaScriptPredicateFactory(function, extractionFn); + predicateFactory = syncedFnPredicateFactory; } } } From fb5eaa887eef896a6ff84703f98a89577bbc53ce Mon Sep 17 00:00:00 2001 From: kamaci Date: Mon, 3 Dec 2018 23:44:40 +0300 Subject: [PATCH 2/6] @Nullable is removed since there is no need to use along with @MonotonicNonNull. --- .../druid/query/aggregation/JavaScriptAggregatorFactory.java | 2 -- .../druid/query/aggregation/post/JavaScriptPostAggregator.java | 2 -- .../apache/druid/query/extraction/JavaScriptExtractionFn.java | 1 - .../java/org/apache/druid/query/filter/JavaScriptDimFilter.java | 2 -- 4 files changed, 7 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java index acfb164258e1..cd55de6e30e5 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java @@ -41,7 +41,6 @@ import org.mozilla.javascript.Function; import org.mozilla.javascript.ScriptableObject; -import javax.annotation.Nullable; import java.lang.reflect.Array; import java.nio.ByteBuffer; import java.security.MessageDigest; @@ -69,7 +68,6 @@ public class JavaScriptAggregatorFactory extends AggregatorFactory * on the fields of the created object */ @MonotonicNonNull - @Nullable private volatile ScriptAggregator compiledScript; @JsonCreator diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/post/JavaScriptPostAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/post/JavaScriptPostAggregator.java index 3d5d3860b057..a59b67817cdf 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/post/JavaScriptPostAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/post/JavaScriptPostAggregator.java @@ -34,7 +34,6 @@ import org.mozilla.javascript.ContextFactory; import org.mozilla.javascript.ScriptableObject; -import javax.annotation.Nullable; import java.util.Comparator; import java.util.List; import java.util.Map; @@ -96,7 +95,6 @@ public double apply(Object[] args) * on the fields of the created object */ @MonotonicNonNull - @Nullable private volatile Function fn; @JsonCreator diff --git a/processing/src/main/java/org/apache/druid/query/extraction/JavaScriptExtractionFn.java b/processing/src/main/java/org/apache/druid/query/extraction/JavaScriptExtractionFn.java index 8dba7a29ccb8..ddbd8dc05ec0 100644 --- a/processing/src/main/java/org/apache/druid/query/extraction/JavaScriptExtractionFn.java +++ b/processing/src/main/java/org/apache/druid/query/extraction/JavaScriptExtractionFn.java @@ -77,7 +77,6 @@ public String apply(Object input) * on the fields of the created object */ @MonotonicNonNull - @Nullable private volatile Function fn; @JsonCreator diff --git a/processing/src/main/java/org/apache/druid/query/filter/JavaScriptDimFilter.java b/processing/src/main/java/org/apache/druid/query/filter/JavaScriptDimFilter.java index 6fa06fa1d319..6d31ec60f2e7 100644 --- a/processing/src/main/java/org/apache/druid/query/filter/JavaScriptDimFilter.java +++ b/processing/src/main/java/org/apache/druid/query/filter/JavaScriptDimFilter.java @@ -36,7 +36,6 @@ import org.mozilla.javascript.Function; import org.mozilla.javascript.ScriptableObject; -import javax.annotation.Nullable; import java.nio.ByteBuffer; import java.util.HashSet; @@ -53,7 +52,6 @@ public class JavaScriptDimFilter implements DimFilter * on the fields of the created object */ @MonotonicNonNull - @Nullable private volatile JavaScriptPredicateFactory predicateFactory; @JsonCreator From 14d9689bc0ad72a379c8869222067bf84d54cc52 Mon Sep 17 00:00:00 2001 From: kamaci Date: Tue, 4 Dec 2018 00:21:37 +0300 Subject: [PATCH 3/6] Static import is removed. --- .../aggregation/JavaScriptAggregatorFactory.java | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java index cd55de6e30e5..b7124629343c 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java @@ -51,8 +51,6 @@ import java.util.Objects; import java.util.stream.Collectors; -import static org.apache.druid.query.aggregation.JavaScriptAggregator.ScriptAggregator; - public class JavaScriptAggregatorFactory extends AggregatorFactory { private final String name; @@ -67,8 +65,7 @@ public class JavaScriptAggregatorFactory extends AggregatorFactory * in {@link #compileScript(String, String, String)} without worrying about final modifiers * on the fields of the created object */ - @MonotonicNonNull - private volatile ScriptAggregator compiledScript; + private volatile JavaScriptAggregator.@MonotonicNonNull ScriptAggregator compiledScript; @JsonCreator public JavaScriptAggregatorFactory( @@ -293,13 +290,13 @@ public String toString() * script compilation. */ @EnsuresNonNull("compiledScript") - private ScriptAggregator getCompiledScript() + private JavaScriptAggregator.ScriptAggregator getCompiledScript() { // 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"); - ScriptAggregator syncedCompiledScript = compiledScript; + JavaScriptAggregator.ScriptAggregator syncedCompiledScript = compiledScript; if (syncedCompiledScript == null) { synchronized (config) { syncedCompiledScript = compiledScript; @@ -313,7 +310,7 @@ private ScriptAggregator getCompiledScript() } @VisibleForTesting - static ScriptAggregator compileScript( + static JavaScriptAggregator.ScriptAggregator compileScript( final String aggregate, final String reset, final String combine @@ -330,7 +327,7 @@ static ScriptAggregator compileScript( final Function fnCombine = context.compileFunction(scope, combine, "combine", 1, null); Context.exit(); - return new ScriptAggregator() + return new JavaScriptAggregator.ScriptAggregator() { @Override public double aggregate(final double current, final BaseObjectColumnValueSelector[] selectorList) From 0814d5b6ecf225acfffbb0276005f3e4e53902db Mon Sep 17 00:00:00 2001 From: kamaci Date: Tue, 4 Dec 2018 16:12:00 +0300 Subject: [PATCH 4/6] Lazy initialization is implemented. --- .../aws/LazyFileSessionCredentialsProvider.java | 8 +++++--- .../aggregation/JavaScriptAggregatorFactory.java | 11 +++++++---- .../aggregation/post/JavaScriptPostAggregator.java | 8 ++++++-- .../query/extraction/JavaScriptExtractionFn.java | 5 +++-- .../druid/query/filter/JavaScriptDimFilter.java | 5 +++-- 5 files changed, 24 insertions(+), 13 deletions(-) diff --git a/aws-common/src/main/java/org/apache/druid/common/aws/LazyFileSessionCredentialsProvider.java b/aws-common/src/main/java/org/apache/druid/common/aws/LazyFileSessionCredentialsProvider.java index 450e77435922..483b32a777a0 100644 --- a/aws-common/src/main/java/org/apache/druid/common/aws/LazyFileSessionCredentialsProvider.java +++ b/aws-common/src/main/java/org/apache/druid/common/aws/LazyFileSessionCredentialsProvider.java @@ -22,8 +22,7 @@ import com.amazonaws.auth.AWSCredentials; import com.amazonaws.auth.AWSCredentialsProvider; import org.checkerframework.checker.nullness.qual.EnsuresNonNull; - -import javax.annotation.Nullable; +import org.checkerframework.checker.nullness.qual.MonotonicNonNull; public class LazyFileSessionCredentialsProvider implements AWSCredentialsProvider { @@ -33,8 +32,11 @@ public class LazyFileSessionCredentialsProvider implements AWSCredentialsProvide * The field is declared volatile in order to ensure safe publication of the object * in {@link #getUnderlyingProvider()} without worrying about final modifiers * on the fields of the created object + * + * @see + * https://github.com/apache/incubator-druid/pull/6662#discussion_r237013157 */ - @Nullable + @MonotonicNonNull private volatile FileSessionCredentialsProvider provider; public LazyFileSessionCredentialsProvider(AWSCredentialsConfig config) diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java index b7124629343c..c98f7ba3ac47 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java @@ -64,6 +64,9 @@ public class JavaScriptAggregatorFactory extends AggregatorFactory * The field is declared volatile in order to ensure safe publication of the object * in {@link #compileScript(String, String, String)} without worrying about final modifiers * on the fields of the created object + * + * @see + * https://github.com/apache/incubator-druid/pull/6662#discussion_r237013157 */ private volatile JavaScriptAggregator.@MonotonicNonNull ScriptAggregator compiledScript; @@ -95,7 +98,7 @@ public JavaScriptAggregatorFactory( @Override public Aggregator factorize(final ColumnSelectorFactory columnFactory) { - getCompiledScript(); + compiledScript = getCompiledScript(); return new JavaScriptAggregator( fieldNames.stream().map(columnFactory::makeColumnValueSelector).collect(Collectors.toList()), compiledScript @@ -105,7 +108,7 @@ public Aggregator factorize(final ColumnSelectorFactory columnFactory) @Override public BufferAggregator factorizeBuffered(final ColumnSelectorFactory columnSelectorFactory) { - getCompiledScript(); + compiledScript = getCompiledScript(); return new JavaScriptBufferAggregator( fieldNames.stream().map(columnSelectorFactory::makeColumnValueSelector).collect(Collectors.toList()), compiledScript @@ -121,7 +124,7 @@ public Comparator getComparator() @Override public Object combine(Object lhs, Object rhs) { - getCompiledScript(); + compiledScript = getCompiledScript(); return compiledScript.combine(((Number) lhs).doubleValue(), ((Number) rhs).doubleValue()); } @@ -141,7 +144,7 @@ public void reset(ColumnValueSelector selector) @Override public void fold(ColumnValueSelector selector) { - getCompiledScript(); + compiledScript = getCompiledScript(); combined = compiledScript.combine(combined, selector.getDouble()); } diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/post/JavaScriptPostAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/post/JavaScriptPostAggregator.java index a59b67817cdf..e47f1c13df77 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/post/JavaScriptPostAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/post/JavaScriptPostAggregator.java @@ -93,6 +93,9 @@ public double apply(Object[] args) * The field is declared volatile in order to ensure safe publication of the object * in {@link #compile(String)} without worrying about final modifiers * on the fields of the created object + * + * @see + * https://github.com/apache/incubator-druid/pull/6662#discussion_r237013157 */ @MonotonicNonNull private volatile Function fn; @@ -130,7 +133,7 @@ public Comparator getComparator() @Override public Object compute(Map combinedAggregators) { - checkAndCompileScript(); + fn = getCompiledScript(); final Object[] args = new Object[fieldNames.size()]; int i = 0; for (String field : fieldNames) { @@ -144,7 +147,7 @@ public Object compute(Map combinedAggregators) * script compilation. */ @EnsuresNonNull("fn") - private void checkAndCompileScript() + private Function getCompiledScript() { // 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. @@ -160,6 +163,7 @@ private void checkAndCompileScript() } } } + return syncedFn; } @Override diff --git a/processing/src/main/java/org/apache/druid/query/extraction/JavaScriptExtractionFn.java b/processing/src/main/java/org/apache/druid/query/extraction/JavaScriptExtractionFn.java index ddbd8dc05ec0..0827ba889b6e 100644 --- a/processing/src/main/java/org/apache/druid/query/extraction/JavaScriptExtractionFn.java +++ b/processing/src/main/java/org/apache/druid/query/extraction/JavaScriptExtractionFn.java @@ -119,7 +119,7 @@ public byte[] getCacheKey() @Nullable public String apply(@Nullable Object value) { - checkAndCompileScript(); + fn = getCompiledScript(); return NullHandling.emptyToNullIfNeeded(fn.apply(value)); } @@ -128,7 +128,7 @@ public String apply(@Nullable Object value) * script compilation. */ @EnsuresNonNull("fn") - private void checkAndCompileScript() + private Function getCompiledScript() { // 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. @@ -144,6 +144,7 @@ private void checkAndCompileScript() } } } + return syncedFn; } @Override diff --git a/processing/src/main/java/org/apache/druid/query/filter/JavaScriptDimFilter.java b/processing/src/main/java/org/apache/druid/query/filter/JavaScriptDimFilter.java index 6d31ec60f2e7..054d126fc254 100644 --- a/processing/src/main/java/org/apache/druid/query/filter/JavaScriptDimFilter.java +++ b/processing/src/main/java/org/apache/druid/query/filter/JavaScriptDimFilter.java @@ -114,7 +114,7 @@ public DimFilter optimize() @Override public Filter toFilter() { - checkAndCreatePredicateFactory(); + predicateFactory = getPredicateFactory(); return new JavaScriptFilter(dimension, predicateFactory); } @@ -123,7 +123,7 @@ public Filter toFilter() * script compilation. */ @EnsuresNonNull("predicateFactory") - private void checkAndCreatePredicateFactory() + private JavaScriptPredicateFactory getPredicateFactory() { // 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. @@ -139,6 +139,7 @@ private void checkAndCreatePredicateFactory() } } } + return syncedFnPredicateFactory; } @Override From abbfab594fcbcf90009df2a9c9463974f8390abc Mon Sep 17 00:00:00 2001 From: kamaci Date: Thu, 6 Dec 2018 17:26:12 +0300 Subject: [PATCH 5/6] Local variables used instead of volatile ones. --- .../druid/query/aggregation/JavaScriptAggregatorFactory.java | 2 +- .../query/aggregation/post/JavaScriptPostAggregator.java | 2 +- .../druid/query/extraction/JavaScriptExtractionFn.java | 3 +++ .../org/apache/druid/query/filter/JavaScriptDimFilter.java | 5 ++++- 4 files changed, 9 insertions(+), 3 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java index c98f7ba3ac47..c3fef9af69fe 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java @@ -98,7 +98,7 @@ public JavaScriptAggregatorFactory( @Override public Aggregator factorize(final ColumnSelectorFactory columnFactory) { - compiledScript = getCompiledScript(); + JavaScriptAggregator.ScriptAggregator compiledScript = getCompiledScript(); return new JavaScriptAggregator( fieldNames.stream().map(columnFactory::makeColumnValueSelector).collect(Collectors.toList()), compiledScript diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/post/JavaScriptPostAggregator.java b/processing/src/main/java/org/apache/druid/query/aggregation/post/JavaScriptPostAggregator.java index e47f1c13df77..1ed1318fd88b 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/post/JavaScriptPostAggregator.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/post/JavaScriptPostAggregator.java @@ -133,7 +133,7 @@ public Comparator getComparator() @Override public Object compute(Map combinedAggregators) { - fn = getCompiledScript(); + Function fn = getCompiledScript(); final Object[] args = new Object[fieldNames.size()]; int i = 0; for (String field : fieldNames) { diff --git a/processing/src/main/java/org/apache/druid/query/extraction/JavaScriptExtractionFn.java b/processing/src/main/java/org/apache/druid/query/extraction/JavaScriptExtractionFn.java index 0827ba889b6e..0db3a26b357b 100644 --- a/processing/src/main/java/org/apache/druid/query/extraction/JavaScriptExtractionFn.java +++ b/processing/src/main/java/org/apache/druid/query/extraction/JavaScriptExtractionFn.java @@ -75,6 +75,9 @@ public String apply(Object input) * The field is declared volatile in order to ensure safe publication of the object * in {@link #compile(String)} without worrying about final modifiers * on the fields of the created object + * + * @see + * https://github.com/apache/incubator-druid/pull/6662#discussion_r237013157 */ @MonotonicNonNull private volatile Function fn; diff --git a/processing/src/main/java/org/apache/druid/query/filter/JavaScriptDimFilter.java b/processing/src/main/java/org/apache/druid/query/filter/JavaScriptDimFilter.java index 054d126fc254..b211e38ad78d 100644 --- a/processing/src/main/java/org/apache/druid/query/filter/JavaScriptDimFilter.java +++ b/processing/src/main/java/org/apache/druid/query/filter/JavaScriptDimFilter.java @@ -50,6 +50,9 @@ public class JavaScriptDimFilter implements DimFilter * The field is declared volatile in order to ensure safe publication of the object * in {@link JavaScriptPredicateFactory(String, ExtractionFn)} without worrying about final modifiers * on the fields of the created object + * + * @see + * https://github.com/apache/incubator-druid/pull/6662#discussion_r237013157 */ @MonotonicNonNull private volatile JavaScriptPredicateFactory predicateFactory; @@ -114,7 +117,7 @@ public DimFilter optimize() @Override public Filter toFilter() { - predicateFactory = getPredicateFactory(); + JavaScriptPredicateFactory predicateFactory = getPredicateFactory(); return new JavaScriptFilter(dimension, predicateFactory); } From 96413231a90b3472d43098dfcca884ce4a183b86 Mon Sep 17 00:00:00 2001 From: kamaci Date: Thu, 6 Dec 2018 21:41:36 +0300 Subject: [PATCH 6/6] Local variables used instead of volatile ones. --- .../query/aggregation/JavaScriptAggregatorFactory.java | 6 +++--- .../druid/query/extraction/JavaScriptExtractionFn.java | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java b/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java index c3fef9af69fe..b5e9ed173eee 100644 --- a/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java +++ b/processing/src/main/java/org/apache/druid/query/aggregation/JavaScriptAggregatorFactory.java @@ -108,7 +108,7 @@ public Aggregator factorize(final ColumnSelectorFactory columnFactory) @Override public BufferAggregator factorizeBuffered(final ColumnSelectorFactory columnSelectorFactory) { - compiledScript = getCompiledScript(); + JavaScriptAggregator.ScriptAggregator compiledScript = getCompiledScript(); return new JavaScriptBufferAggregator( fieldNames.stream().map(columnSelectorFactory::makeColumnValueSelector).collect(Collectors.toList()), compiledScript @@ -124,7 +124,7 @@ public Comparator getComparator() @Override public Object combine(Object lhs, Object rhs) { - compiledScript = getCompiledScript(); + JavaScriptAggregator.ScriptAggregator compiledScript = getCompiledScript(); return compiledScript.combine(((Number) lhs).doubleValue(), ((Number) rhs).doubleValue()); } @@ -144,7 +144,7 @@ public void reset(ColumnValueSelector selector) @Override public void fold(ColumnValueSelector selector) { - compiledScript = getCompiledScript(); + JavaScriptAggregator.ScriptAggregator compiledScript = getCompiledScript(); combined = compiledScript.combine(combined, selector.getDouble()); } diff --git a/processing/src/main/java/org/apache/druid/query/extraction/JavaScriptExtractionFn.java b/processing/src/main/java/org/apache/druid/query/extraction/JavaScriptExtractionFn.java index 0db3a26b357b..3b95b7507b15 100644 --- a/processing/src/main/java/org/apache/druid/query/extraction/JavaScriptExtractionFn.java +++ b/processing/src/main/java/org/apache/druid/query/extraction/JavaScriptExtractionFn.java @@ -122,7 +122,7 @@ public byte[] getCacheKey() @Nullable public String apply(@Nullable Object value) { - fn = getCompiledScript(); + Function fn = getCompiledScript(); return NullHandling.emptyToNullIfNeeded(fn.apply(value)); }