Require combine and reduce scripts in scripted metrics aggregation #33452

albendz · 2018-09-06T03:54:58Z

Fixes #32804

Implementation notes:

Integration and unit tests updated
Throws IllegalArgumentException when combine or reduce is not provided
only unit tests test for exceptions, integration only tests positive cases
Behavior:
no combine, no reduce -> exception
combine, no reduce -> exception
no combine, reduce -> exception
combine, reduce -> success

…e and combine

elasticmachine · 2018-09-06T08:10:01Z

Pinging @elastic/es-search-aggs

albendz · 2018-09-10T19:18:04Z

Since first creating this there is now a conflict. Do I need to resolve the conflicts before anyone takes a look?

colings86

@albendz I left a small comment but I think this looks pretty good. If you could address that and update the branch with the latest master I'll kick of a CI build

colings86 · 2018-09-10T20:40:29Z

...org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregationBuilder.java

+            throw new IllegalArgumentException("[combineScript] must not be null: [" + name + "]");
+        }
+
+        if(reduceScript == null) {


Could we do these null checks at the start of the method before we compile any scripts?

albendz · 2018-09-12T04:06:52Z

@colings86 I tried moving the null checks but it changed the test behavior. The init scripts might be setting those values so the order might need to stay afterwards. It takes some time to rerun the tests so I'll keep trying to figure this out but it will take a while.

colings86 · 2018-09-12T08:45:42Z

...ain/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregationBuilder.java

+            compiledCombineScript = queryShardContext.getScriptService().compile(combineScript,
+                ScriptedMetricAggContexts.CombineScript.CONTEXT);
+            combineScriptParams = combineScript.getParams();
+        } else {


I think we should leave lines 204-206 here where they were below but just have the null check here so here its just:

if (combineScript == null) { throw new IllegalArgumentException("[combineScript] must not be null: [" + name + "]"); }

And then below after the map script parts we have:

ScriptedMetricAggContexts.CombineScript.Factory compiledCombineScript = queryShardContext.getScriptService().compile(combineScript, ScriptedMetricAggContexts.CombineScript.CONTEXT); Map<String, Object> combineScriptParams = combineScript.getParams();

Making this change causes all the integ tests to fail and I haven't been able to debug why yet.

all = all the scripted metric IT integ tests

As I said, this combination fails the integration tests and I'm not sure why. It seems the return values of the test scripts are returning different values if done this way.

So, I'm currently debugging issues with the dummy scripts interfering with return values. It takes a while since they are integ tests. Just letting you know I'm still working on this.

…rect test scripts in IT

albendz · 2018-09-16T03:44:26Z

Fixed - integ tests should have failed for my initial script but for whatever reason they did not (possibly because of a failed clean task).

colings86 · 2018-09-25T10:11:55Z

@elasticmachine test this please

colings86 · 2018-09-26T08:01:29Z

@albendz it looks like the build for an unrelated reason. Would you mind updating the branch with the latest master and I'll set the tests off again?

albendz · 2018-09-27T19:06:35Z

I've updated the branch. Some unrelated tests failed but that might be slowness from my machine.

colings86 · 2018-09-28T07:39:18Z

@elasticmachine test this please

colings86 · 2018-10-01T12:39:30Z

@albendz I think this change looks good now. However I have forgotten about two things that will need to be done before this can be merged:

We should add a note in the breaking changes documentation so users upgrading to 7.0.0 can see that this behaviour has changed
Although this breaking change will happen in 7.0.0 we should prepare a PR for the 6.x branch which instead of throwing an exception logs a warning with the deprecation logger so if users on later 6.x release do not specify a combine or reduce script when using the scripted_metric aggregation they will be notified that their scripted metric aggregation will not work

Are you happy to make these changes? Sorry for not raising this to you before, doing this two things had slipped my mind.

albendz · 2018-10-01T23:55:50Z

I will try to get started on these changes but I will be unavailable for 3 weeks starting Oct 4. I'll see how far I get!

albendz · 2018-10-02T22:50:01Z

Can you give me an example of what using the deprecation logger looks like?

albendz · 2018-10-03T04:44:57Z

Created 6.x PR: #34254

colings86

Thanks for the work and for creating the 6.x version of this change. providing the current test runs succeed I'll go ahead an merge these changes

colings86 · 2018-10-03T08:23:33Z

@elasticmachine test this please

colings86 · 2018-10-03T14:23:21Z

@albendz thanks for working on this, I've merged this change into master and 6.x

When combine_script and reduce_script were made into required parameters for Scripted Metric aggregations in #33452, the docs were not updated to reflect that. This marks those parameters as required in the documentation.

…33452) * Make text message not required in constructor for slack * Remove unnecessary comments in test file * Throw exception when reduce or combine is not provided; update tests * Update integration tests for scripted metrics to always include reduce and combine * Remove some old changes from previous branches * Rearrange script presence checks to be earlier in build * Change null check order in script builder for aggregated metrics; correct test scripts in IT * Add breaking change details to PR

When combine_script and reduce_script were made into required parameters for Scripted Metric aggregations in #33452, the docs were not updated to reflect that. This marks those parameters as required in the documentation.

albendz added 9 commits June 12, 2018 19:49

Make text message not required in constructor for slack

9c8e69c

Remove unnecessary comments in test file

b6876b4

Merge remote-tracking branch 'upstream/master'

51ff0a0

Merge remote-tracking branch 'upstream/master'

5df7b98

Merge remote-tracking branch 'upstream/master'

156abbf

Merge remote-tracking branch 'upstream/master'

68fdddc

Throw exception when reduce or combine is not provided; update tests

63bf29a

Update integration tests for scripted metrics to always include reduc…

f68326e

…e and combine

Remove some old changes from previous branches

3da8fd5

tlrx added the :Analytics/Aggregations Aggregations label Sep 6, 2018

colings86 reviewed Sep 10, 2018

View reviewed changes

albendz added 2 commits September 11, 2018 10:19

Merge remote-tracking branch 'upstream/master'

78f7a2e

Merge branch 'master' into require_scripts

00050da

Rearrange script presence checks to be earlier in build

667557a

colings86 reviewed Sep 12, 2018

View reviewed changes

albendz added 2 commits September 15, 2018 20:16

Change null check order in script builder for aggregated metrics; cor…

1adb4ed

…rect test scripts in IT

Merge remote-tracking branch 'upstream/master' into require_scripts

8db9925

albendz added 2 commits September 27, 2018 11:14

Merge remote-tracking branch 'upstream/master'

b119cbd

Merge remote-tracking branch 'origin' into require_scripts

e451b84

Add breaking change details to PR

120bab7

albendz added 2 commits October 2, 2018 15:31

Merge remote-tracking branch 'upstream/master'

1495c24

Merge branch 'master' into require_scripts

790e5fd

albendz mentioned this pull request Oct 3, 2018

Port aggregation parameter requirement changes from master branch #34254

Merged

colings86 approved these changes Oct 3, 2018

View reviewed changes

colings86 merged commit f09190c into elastic:master Oct 3, 2018

colings86 added >deprecation v7.0.0 v6.5.0 >breaking and removed >deprecation labels Oct 3, 2018

gwbrown mentioned this pull request Oct 29, 2018

Label required scripts in Scripted Metric Agg docs #35051

Merged

Mpdreamz mentioned this pull request Dec 13, 2018

[meta] 6.5.0 Release elastic/elasticsearch-net#3457

Closed

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require combine and reduce scripts in scripted metrics aggregation #33452

Require combine and reduce scripts in scripted metrics aggregation #33452

albendz commented Sep 6, 2018

elasticmachine commented Sep 6, 2018

albendz commented Sep 10, 2018

colings86 left a comment

colings86 Sep 10, 2018

albendz Sep 10, 2018

albendz commented Sep 12, 2018

colings86 Sep 12, 2018

albendz Sep 12, 2018

albendz Sep 12, 2018

albendz Sep 14, 2018

albendz Sep 15, 2018

albendz commented Sep 16, 2018

colings86 commented Sep 25, 2018

colings86 commented Sep 26, 2018

albendz commented Sep 27, 2018

colings86 commented Sep 28, 2018

colings86 commented Oct 1, 2018

albendz commented Oct 1, 2018

albendz commented Oct 2, 2018

albendz commented Oct 3, 2018

colings86 left a comment

colings86 commented Oct 3, 2018

colings86 commented Oct 3, 2018

Require combine and reduce scripts in scripted metrics aggregation #33452

Require combine and reduce scripts in scripted metrics aggregation #33452

Conversation

albendz commented Sep 6, 2018

elasticmachine commented Sep 6, 2018

albendz commented Sep 10, 2018

colings86 left a comment

Choose a reason for hiding this comment

colings86 Sep 10, 2018

Choose a reason for hiding this comment

albendz Sep 10, 2018

Choose a reason for hiding this comment

albendz commented Sep 12, 2018

colings86 Sep 12, 2018

Choose a reason for hiding this comment

albendz Sep 12, 2018

Choose a reason for hiding this comment

albendz Sep 12, 2018

Choose a reason for hiding this comment

albendz Sep 14, 2018

Choose a reason for hiding this comment

albendz Sep 15, 2018

Choose a reason for hiding this comment

albendz commented Sep 16, 2018

colings86 commented Sep 25, 2018

colings86 commented Sep 26, 2018

albendz commented Sep 27, 2018

colings86 commented Sep 28, 2018

colings86 commented Oct 1, 2018

albendz commented Oct 1, 2018

albendz commented Oct 2, 2018

albendz commented Oct 3, 2018

colings86 left a comment

Choose a reason for hiding this comment

colings86 commented Oct 3, 2018

colings86 commented Oct 3, 2018