Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Scripted metric aggregations: add deprecation warning and system property to control legacy params #31597

Merged
merged 17 commits into from
Aug 17, 2018
Merged

Scripted metric aggregations: add deprecation warning and system property to control legacy params #31597

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
b9111d0
Scripted metric aggregations: add deprecation warning and system prop…
rationull Jun 27, 2018
c7a6ad1
Merge branch 'master' into rationull/metric-agg-context-deprecation-a…
rationull Jul 12, 2018
bc86682
Fix minor style issue and docs test failure
rationull Jul 13, 2018
e49f8f7
Merge branch 'master' into rationull/metric-agg-context-deprecation-a…
rationull Jul 14, 2018
7fd0484
Disable deprecated params._agg/_aggs in tests and revise tests to use…
rationull Jul 18, 2018
8185ac3
Add integration test covering deprecated scripted metrics aggs params…
rationull Jul 18, 2018
9f0b28b
Disable deprecated params._agg/_aggs in docs integration tests and re…
rationull Jul 18, 2018
bf13bf4
Revert unnecessary migrations doc change
rationull Jul 19, 2018
9681738
Replace deprecated _agg param bwc integration test with a couple of u…
rationull Jul 20, 2018
da54990
Merge branch 'master' into rationull/metric-agg-context-deprecation-a…
rationull Jul 20, 2018
20ac8d8
Fix compatibility test after merge
rationull Jul 20, 2018
3ea60ee
Rename backwards compatibility system property per code review feedback
rationull Aug 2, 2018
aa2e066
Merge branch 'master' into rationull/metric-agg-context-deprecation-a…
rationull Aug 2, 2018
6009712
Tweak deprecation warning text per review feedback
rationull Aug 9, 2018
30ff793
Merge branch 'master' into rationull/metric-agg-context-deprecation-a…
rationull Aug 9, 2018
22f74ed
Merge branch 'master' into rationull/metric-agg-context-deprecation-a…
rationull Aug 14, 2018
087c005
Merge branch 'master' into rationull/metric-agg-context-deprecation-a…
rationull Aug 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/reference/migration/migrate_7_0/aggregations.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,5 @@ The object used to share aggregation state between the scripts in a Scripted Met
Aggregation is now a variable called `state` available in the script context, rather than
being provided via the `params` object as `params._agg`.

The old `params._agg` variable is still available as well.
The old `params._agg` variable is still available as well. This can be disabled by setting
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this will be backported to 6.x, with the intention of removing in 7.0, I don't think this migration note makes sense. The migration note would be that params._agg is gone, and would be in the followup which removes support in master only.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks. I overthought this and was going to remove it in my followup, but you're right; there's no need for this note here now that the target version is more solid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll revert the doc change such that this file is left as-is until the next PR.

the `es.aggregations.enableDeprecatedScriptedMetricAggParam` system property to `false`.
21 changes: 21 additions & 0 deletions server/src/main/java/org/elasticsearch/script/ScriptedMetricAggContexts.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import org.apache.lucene.index.LeafReaderContext;
import org.apache.lucene.search.Scorer;
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.logging.Loggers;
import org.elasticsearch.index.fielddata.ScriptDocValues;
import org.elasticsearch.search.lookup.LeafSearchLookup;
import org.elasticsearch.search.lookup.SearchLookup;
Expand All @@ -31,6 +33,25 @@
import java.util.Map;

public class ScriptedMetricAggContexts {
private static final DeprecationLogger DEPRECATION_LOGGER =
new DeprecationLogger(Loggers.getLogger(ScriptedMetricAggContexts.class));

// Public for access from tests
public static final String AGG_PARAM_DEPRECATION_WARNING =
"params._agg/_aggs for scripted metric aggregations are deprecated, use state/states (not in params) instead. " +
"Set system property es.aggregations.enableDeprecatedScriptedMetricAggParam = false to disable this deprecated behavior.";

public static boolean deprecatedAggParamEnabled() {
boolean enabled = Boolean.parseBoolean(
System.getProperty("es.aggregations.enableDeprecatedScriptedMetricAggParam", "true"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be named with underscores. I think the name doesn't need the "deprecated" in it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So enable_scripted_metric_agg_param or is that too generic?


if (enabled) {
DEPRECATION_LOGGER.deprecatedAndMaybeLog("enableDeprecatedScriptedMetricAggParams", AGG_PARAM_DEPRECATION_WARNING);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I've lead you in an incorrect direction. After discussing with others, we want these to only be triggered when the deprecated behavior is used. See #31441 for and example doing this in ScriptDocValues for an idea of what must be done within the aggs code (although I think it will be simpler here, since you can do this check and deprecation outside of script invocation, and thus no need for a doPriv block).

}

return enabled;
}

private abstract static class ParamsAndStateBase {
private final Map<String, Object> params;
private final Object state;
Expand Down
4 changes: 3 additions & 1 deletion ...n/java/org/elasticsearch/search/aggregations/metrics/scripted/InternalScriptedMetric.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,9 @@ public InternalAggregation doReduce(List<InternalAggregation> aggregations, Redu
}

// Add _aggs to params map for backwards compatibility (redundant with a context variable on the ReduceScript created below).
params.put("_aggs", aggregationObjects);
if (ScriptedMetricAggContexts.deprecatedAggParamEnabled()) {
params.put("_aggs", aggregationObjects);
}

ScriptedMetricAggContexts.ReduceScript.Factory factory = reduceContext.scriptService().compile(
firstAggregation.reduceScript, ScriptedMetricAggContexts.ReduceScript.CONTEXT);
Expand Down
13 changes: 10 additions & 3 deletions ...g/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregatorFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,17 @@ public Aggregator createInternal(Aggregator parent, boolean collectsFromSingleBu
// Add _agg to params map for backwards compatibility (redundant with context variables on the scripts created below).
// When this is removed, aggState (as passed to ScriptedMetricAggregator) can be changed to Map<String, Object>, since
// it won't be possible to completely replace it with another type as is possible when it's an entry in params.
if (aggParams.containsKey("_agg") == false) {
aggParams.put("_agg", new HashMap<String, Object>());
Object aggState = new HashMap<String, Object>();
if (ScriptedMetricAggContexts.deprecatedAggParamEnabled()) {
if (!aggParams.containsKey("_agg")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we tend to prefer aggParams.containsKey("_agg") == false as its more obvious to read since ! can easily be missed

// Add _agg if it wasn't added manually
aggParams.put("_agg", aggState);
} else {
// If it was added manually, also use it for the agg context variable to reduce the likelihood of
// weird behavior due to multiple different variables.
aggState = aggParams.get("_agg");
}
}
Object aggState = aggParams.get("_agg");

final ScriptedMetricAggContexts.InitScript initScript = this.initScript.newInstance(
mergeParams(aggParams, initScriptParams), aggState);
Expand Down
2 changes: 2 additions & 0 deletions ...a/org/elasticsearch/search/aggregations/metrics/scripted/InternalScriptedMetricTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.script.MockScriptEngine;
import org.elasticsearch.script.Script;
import org.elasticsearch.script.ScriptedMetricAggContexts;
import org.elasticsearch.script.ScriptEngine;
import org.elasticsearch.script.ScriptModule;
import org.elasticsearch.script.ScriptService;
Expand Down Expand Up @@ -129,6 +130,7 @@ protected void assertReduced(InternalScriptedMetric reduced, List<InternalScript
assertEquals(firstAgg.getMetaData(), reduced.getMetaData());
if (hasReduceScript) {
assertEquals(inputs.size(), reduced.aggregation());
assertWarnings(ScriptedMetricAggContexts.AGG_PARAM_DEPRECATION_WARNING);
} else {
assertEquals(inputs.size(), ((List<Object>) reduced.aggregation()).size());
}
Expand Down
19 changes: 19 additions & 0 deletions ...org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregatorTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import org.elasticsearch.indices.breaker.CircuitBreakerService;
import org.elasticsearch.script.MockScriptEngine;
import org.elasticsearch.script.Script;
import org.elasticsearch.script.ScriptedMetricAggContexts;
import org.elasticsearch.script.ScriptEngine;
import org.elasticsearch.script.ScriptModule;
import org.elasticsearch.script.ScriptService;
Expand Down Expand Up @@ -169,6 +170,8 @@ public void testNoDocs() throws IOException {
assertEquals(0, ((HashMap<Object, String>) scriptedMetric.aggregation()).size());
}
}

assertWarnings(ScriptedMetricAggContexts.AGG_PARAM_DEPRECATION_WARNING);
}

/**
Expand All @@ -195,6 +198,8 @@ public void testScriptedMetricWithoutCombine() throws IOException {
assertEquals(numDocs, list.size());
}
}

assertWarnings(ScriptedMetricAggContexts.AGG_PARAM_DEPRECATION_WARNING);
}

/**
Expand All @@ -217,6 +222,8 @@ public void testScriptedMetricWithCombine() throws IOException {
assertEquals(numDocs, scriptedMetric.aggregation());
}
}

assertWarnings(ScriptedMetricAggContexts.AGG_PARAM_DEPRECATION_WARNING);
}

/**
Expand All @@ -240,6 +247,8 @@ public void testScriptedMetricWithCombineAccessesScores() throws IOException {
assertEquals((double) numDocs, scriptedMetric.aggregation());
}
}

assertWarnings(ScriptedMetricAggContexts.AGG_PARAM_DEPRECATION_WARNING);
}

public void testScriptParamsPassedThrough() throws IOException {
Expand All @@ -259,6 +268,8 @@ public void testScriptParamsPassedThrough() throws IOException {
assertEquals(306, scriptedMetric.aggregation());
}
}

assertWarnings(ScriptedMetricAggContexts.AGG_PARAM_DEPRECATION_WARNING);
}

public void testConflictingAggAndScriptParams() throws IOException {
Expand All @@ -282,6 +293,8 @@ public void testConflictingAggAndScriptParams() throws IOException {
ex.getMessage());
}
}

assertWarnings(ScriptedMetricAggContexts.AGG_PARAM_DEPRECATION_WARNING);
}

public void testSelfReferencingAggStateAfterInit() throws IOException {
Expand All @@ -299,6 +312,8 @@ public void testSelfReferencingAggStateAfterInit() throws IOException {
assertEquals("Iterable object is self-referencing itself (Scripted metric aggs init script)", ex.getMessage());
}
}

assertWarnings(ScriptedMetricAggContexts.AGG_PARAM_DEPRECATION_WARNING);
}

public void testSelfReferencingAggStateAfterMap() throws IOException {
Expand All @@ -319,6 +334,8 @@ public void testSelfReferencingAggStateAfterMap() throws IOException {
assertEquals("Iterable object is self-referencing itself (Scripted metric aggs map script)", ex.getMessage());
}
}

assertWarnings(ScriptedMetricAggContexts.AGG_PARAM_DEPRECATION_WARNING);
}

public void testSelfReferencingAggStateAfterCombine() throws IOException {
Expand All @@ -336,6 +353,8 @@ public void testSelfReferencingAggStateAfterCombine() throws IOException {
assertEquals("Iterable object is self-referencing itself (Scripted metric aggs combine script)", ex.getMessage());
}
}

assertWarnings(ScriptedMetricAggContexts.AGG_PARAM_DEPRECATION_WARNING);
}

/**
Expand Down