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.

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

Remove support for deprecated params._agg/_aggs for scripted metric aggregations #32979

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -798,8 +798,6 @@ class BuildPlugin implements Plugin<Project> {
systemProperty 'tests.task', path
systemProperty 'tests.security.manager', 'true'
systemProperty 'jna.nosys', 'true'
// TODO: remove this deprecation compatibility setting for 7.0
systemProperty 'es.aggregations.enable_scripted_metric_agg_param', 'false'
systemProperty 'compiler.java', project.ext.compilerJavaVersion.getMajorVersion()
if (project.ext.inFipsJvm) {
systemProperty 'runtime.java', project.ext.runtimeJavaVersion.getMajorVersion() + "FIPS"
Expand Down
3 changes: 0 additions & 3 deletions docs/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,6 @@ integTestCluster {
// TODO: remove this for 7.0, this exists to allow the doc examples in 6.x to continue using the defaults
systemProperty 'es.scripting.use_java_time', 'false'
systemProperty 'es.scripting.update.ctx_in_params', 'false'

// TODO: remove this deprecation compatibility setting for 7.0
systemProperty 'es.aggregations.enable_scripted_metric_agg_param', 'false'
}

// remove when https://github.com/elastic/elasticsearch/issues/31305 is fixed
Expand Down
2 changes: 0 additions & 2 deletions docs/reference/migration/migrate_7_0/aggregations.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,3 @@ has been removed. `missing_bucket` should be used instead.
The object used to share aggregation state between the scripts in a Scripted Metric
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.
13 changes: 0 additions & 13 deletions server/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -341,16 +341,3 @@ if (isEclipse == false || project.path == ":server-tests") {
check.dependsOn integTest
integTest.mustRunAfter test
}

// TODO: remove these compatibility tests in 7.0
additionalTest('testScriptedMetricAggParamsV6Compatibility') {
include '**/ScriptedMetricAggregatorAggStateV6CompatTests.class'
include '**/InternalScriptedMetricAggStateV6CompatTests.class'
systemProperty 'es.aggregations.enable_scripted_metric_agg_param', 'true'
}

test {
// these are tested explicitly in separate test tasks
exclude '**/ScriptedMetricAggregatorAggStateV6CompatTests.class'
exclude '**/InternalScriptedMetricAggStateV6CompatTests.class'
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@
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 @@ -33,30 +31,11 @@
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. " +
"Use -Des.aggregations.enable_scripted_metric_agg_param=false to disable.";

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

if (enabled) {
DEPRECATION_LOGGER.deprecatedAndMaybeLog("enable_scripted_metric_agg_param", AGG_PARAM_DEPRECATION_WARNING);
}

return enabled;
}

private abstract static class ParamsAndStateBase {
private final Map<String, Object> params;
private final Object state;
private final Map<String, Object> state;

ParamsAndStateBase(Map<String, Object> params, Object state) {
ParamsAndStateBase(Map<String, Object> params, Map<String, Object> state) {
this.params = params;
this.state = state;
}
Expand All @@ -71,14 +50,14 @@ public Object getState() {
}

public abstract static class InitScript extends ParamsAndStateBase {
public InitScript(Map<String, Object> params, Object state) {
public InitScript(Map<String, Object> params, Map<String, Object> state) {
super(params, state);
}

public abstract void execute();

public interface Factory {
InitScript newInstance(Map<String, Object> params, Object state);
InitScript newInstance(Map<String, Object> params, Map<String, Object> state);
}

public static String[] PARAMETERS = {};
Expand All @@ -89,7 +68,7 @@ public abstract static class MapScript extends ParamsAndStateBase {
private final LeafSearchLookup leafLookup;
private Scorer scorer;

public MapScript(Map<String, Object> params, Object state, SearchLookup lookup, LeafReaderContext leafContext) {
public MapScript(Map<String, Object> params, Map<String, Object> state, SearchLookup lookup, LeafReaderContext leafContext) {
super(params, state);

this.leafLookup = leafContext == null ? null : lookup.getLeafSearchLookup(leafContext);
Expand Down Expand Up @@ -131,22 +110,22 @@ public interface LeafFactory {
}

public interface Factory {
LeafFactory newFactory(Map<String, Object> params, Object state, SearchLookup lookup);
LeafFactory newFactory(Map<String, Object> params, Map<String, Object> state, SearchLookup lookup);
}

public static String[] PARAMETERS = new String[] {};
public static ScriptContext<Factory> CONTEXT = new ScriptContext<>("aggs_map", Factory.class);
}

public abstract static class CombineScript extends ParamsAndStateBase {
public CombineScript(Map<String, Object> params, Object state) {
public CombineScript(Map<String, Object> params, Map<String, Object> state) {
super(params, state);
}

public abstract Object execute();

public interface Factory {
CombineScript newInstance(Map<String, Object> params, Object state);
CombineScript newInstance(Map<String, Object> params, Map<String, Object> state);
}

public static String[] PARAMETERS = {};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,6 @@ public InternalAggregation doReduce(List<InternalAggregation> aggregations, Redu
params.putAll(firstAggregation.reduceScript.getParams());
}

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

ScriptedMetricAggContexts.ReduceScript.Factory factory = reduceContext.scriptService().compile(
firstAggregation.reduceScript, ScriptedMetricAggContexts.ReduceScript.CONTEXT);
ScriptedMetricAggContexts.ReduceScript script = factory.newInstance(params, aggregationObjects);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,10 @@ public class ScriptedMetricAggregator extends MetricsAggregator {
private final ScriptedMetricAggContexts.MapScript.LeafFactory mapScript;
private final ScriptedMetricAggContexts.CombineScript combineScript;
private final Script reduceScript;
private Object aggState;
private Map<String, Object> aggState;

protected ScriptedMetricAggregator(String name, ScriptedMetricAggContexts.MapScript.LeafFactory mapScript, ScriptedMetricAggContexts.CombineScript combineScript,
Script reduceScript, Object aggState, SearchContext context, Aggregator parent,
Script reduceScript, Map<String, Object> aggState, SearchContext context, Aggregator parent,
List<PipelineAggregator> pipelineAggregators, Map<String, Object> metaData)
throws IOException {
super(name, context, parent, pipelineAggregators, metaData);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,20 +80,7 @@ public Aggregator createInternal(Aggregator parent, boolean collectsFromSingleBu
aggParams = new HashMap<>();
}

// 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.
Object aggState = new HashMap<String, Object>();
if (ScriptedMetricAggContexts.deprecatedAggParamEnabled()) {
if (aggParams.containsKey("_agg") == false) {
// 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");
}
}
Map<String, Object> aggState = new HashMap<String, Object>();

final ScriptedMetricAggContexts.InitScript initScript = this.initScript.newInstance(
mergeParams(aggParams, initScriptParams), aggState);
Expand Down

This file was deleted.

Loading