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

rationull · 2018-06-27T05:03:38Z

This is a followup to PR #30111 implementing issue #29328.

This implements the suggestion from @rjernst that the legacy params._agg/_aggs should be available by default and disabled via a system property. A deprecation warning is emitted when scripted metric aggregations are used with the deprecated params enabled.

This is to be followed up with another PR removing the deprecated behavior (and updating relevant tests). I have that implemented but will finalize it after making any requested changes here, and presumably it wouldn't get merged until this is ported to the appropriate release branch and a target branch is selected for removing the old params.

…erty to control legacy params Scripted metric aggregation params._agg/_aggs are replaced by state/states context variables. By default the old params are still present, and a deprecation warning is emitted when Scripted Metric Aggregations are used. A new system property can be used to disable the legacy params. This functionality will be removed in a future revision.

elasticmachine · 2018-06-27T06:32:39Z

Pinging @elastic/es-search-aggs

colings86 · 2018-06-28T09:29:38Z

@rationull @rjernst I think I've missed something here. Was it intended that #30111 was backported to 6.x so we can deprecate the use of params._agg/_aggs in 6.x and remove it in 7.0? If so we need to backport #30111 to the 6.x branch as its currently only in master

rationull · 2018-06-28T11:26:10Z

@rjernst's suggestion in a comment on backwards compatibility in #30111 mentioned it may be backported to 6.x, but my assumption was that even if not, the old functionality should be deprecated for a release regardless of of whether that release is 6 or 7. If that's not correct then this can be disregarded and I can submit a PR removing the old functionality with no deprecation step.

(I'm not trying to push a particular schedule here - whatever you all think makes the most sense.)

colings86 · 2018-06-28T11:46:26Z

@rationull you are right that this PR would be needed to deprecate the old functionality in master even if we don't back-port so this PR is definitely needed. The question is whether #30111 should have been back-ported to 6.x.

@rjernst I am on vacation after today so if I should have back-ported #30111 then I am happy either for you to do the back-port of the commit or I can do it when I return. If you could leave a comment either here or on #30111 indicating what should be done that would be great.

rjernst · 2018-06-30T01:18:57Z

Yes, #30111 should be backported to 6.x I will do that next week.

rjernst · 2018-07-03T01:25:26Z

The back port of #30111 is complete.

rationull · 2018-07-04T19:57:36Z

Thanks @rjernst. Then this should be tagged v6.4.0 too presumably?

rjernst · 2018-07-05T15:17:06Z

Yes, I've added the 6.4.0 label.

colings86 · 2018-07-09T08:50:34Z

Thanks for backporting this @rjernst

…nd-disable

rationull · 2018-07-12T05:18:53Z

Updated with latest from master. @colings86 does this look OK to you and would you mind kicking it to Jenkins?

colings86

Thanks for the ping @rationull, this slipped off my list to review following my vacation so the reminder was definitely needed. I left a small comment but I'll set off CI so we can catch any failures in the meantime too

colings86 · 2018-07-12T07:50:50Z

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

-            aggParams.put("_agg", new HashMap<String, Object>());
+        Object aggState = new HashMap<String, Object>();
+        if (ScriptedMetricAggContexts.deprecatedAggParamEnabled()) {
+            if (!aggParams.containsKey("_agg")) {


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

colings86 · 2018-07-12T07:55:30Z

@elasticmachine test this please

rationull · 2018-07-12T14:48:11Z

@colings86 thanks, I will address the comment when I address the test failure. Looks like a trivial failure -- I neglected to add the expected warning headers to the docs.

rationull · 2018-07-13T03:20:10Z

@colings86 fixed the style nit and the test failures. The docs sample test setup is pretty slick!

Would you mind kicking this to CI again?

colings86 · 2018-07-13T07:50:46Z

@elasticmachine test this please

colings86 · 2018-07-13T11:36:12Z

@rationull the latest failure looks unrelated to your changes but would you be able to update your branch with the latest master and I'll kick off CI again to see if we can get a clean run?

rationull · 2018-07-14T04:21:08Z

I agree that this failure looks unrelated. I haven't managed to reproduce it in several runs; I wonder if it's a timing issue that would disappear even with another run as-is?

The master build appears to be broken now due to a conflict between c662565 and 305bfea so I will check back and update my branch when that is fixed.

…nd-disable

rationull · 2018-07-15T05:41:43Z

@colings86 the branch is updated now.

colings86 · 2018-07-16T08:32:49Z

@elasticmachine test this please

colings86 · 2018-07-16T12:09:56Z

docs/reference/aggregations/metrics/scripted-metric-aggregation.asciidoc

@@ -26,6 +26,7 @@ POST ledger/_search?size=0
 --------------------------------------------------
 // CONSOLE
 // TEST[setup:ledger]
+// TEST[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.]


sorry for not seeing this earlier: I think we should make the test cluster set the system property so the deprecated behaviour is not used?

Yes it should be set to the new behavior in all tests, and a specific test added for the bwc behavior. See https://github.com/elastic/elasticsearch/pull/30975/files#diff-407c748a710cb1e33230c1fd06da7f16R694 for how it was done recently for another bwc change (although adding the sysprop in BuildPlugin should really have a comment indicating the purpose and when it can be removed).

Excellent -- thanks for the feedback. I already have a commit in another branch updating the various tests to use the new context variables (PR to follow this one) so I can bring over those changes and add another dedicated test class for the old behavior similar to ScriptDocValuesMissingV6BehaviourTests from that example.

I should be able to get back to this some evening this week. In the meantime to make sure we're on the same page, please let me know if this is different from what you had in mind:

The deprecated behavior will be disabled in general via setting enableDeprecatedScriptedMetricAggParam to false centrally in BuildPlugin.groovy. With a comment that this gets remove presumably in 7.0.

The deprecated behavior will be enabled again for a single dedicated integration test class (because this will get us better coverage across the affected classes vs a unit test).

Some of the test changes are substantial (ScriptedMetricIT specifically) so heads up this review will get bigger.

rjernst · 2018-08-09T17:02:58Z

@elasticmachine ok to test

…nd-disable

rationull · 2018-08-14T06:40:09Z

Merged in the latest from master. Not sure what's going on with the CI failure -- :server:integTest passed for me before submitting (and just now I ran it again too). (I ran it on macOS though). The failure doesn't look related.

rjernst · 2018-08-14T18:31:23Z

I agree it does not look related, and it also does not reproduce for me. Let's try again.

@elasticmachine test this please

rjernst · 2018-08-15T21:32:51Z

Looks like the same failure. @colings86 do you have any ideas? I do not see with that AckIT has to do with scripted metric aggregations.

…nd-disable

rationull · 2018-08-16T06:33:02Z

Merged in master again and the builds have passed this time.

rjernst · 2018-08-16T16:10:54Z

This looks fine from my side now, @colings86 do you want to review?

colings86

LGTM2, I'll merge and backport this now

colings86 · 2018-08-17T12:12:15Z

@rationull thanks again for working on this. Are you able to create a PR for removing the deprecation stuff in master (7.0) as well (the TODOs in the code)?

colings86 · 2018-08-17T13:14:45Z

Backport pending for 6.x branch, waiting for build on #32944

rationull · 2018-08-17T13:27:07Z

@colings86 absolutely -- I'll make a PR for removing the deprecated stuff this weekend.

colings86 · 2018-08-17T13:31:47Z

Awesome, thanks 👍

* elastic/master: (46 commits) NETWORKING: Make RemoteClusterConn. Lazy Resolve DNS (#32764) [DOCS] Splits the users API documentation into multiple pages (#32825) [DOCS] Splits the token APIs into separate pages (#32865) [DOCS] Creates redirects for role management APIs page Bypassing failing test PainlessDomainSplitIT#testHRDSplit (#32966) TEST: Mute testRetentionPolicyChangeDuringRecovery [DOCS] Fixes more broken links to role management APIs [Docs] Tweaks and fixes to rollup docs [DOCS] Fixes links to role management APIs [ML][TEST] Fix BasicRenormalizationIT after adding multibucket feature [DOCS] Splits the roles API documentation into multiple pages (#32794) [TEST] Run pre 6.4 nodes in non-FIPS JVMs (#32901) Make Geo Context Mapping Parsing More Strict (#32821) [ML] fix updating opened jobs scheduled events (#31651) (#32881) Scripted metric aggregations: add deprecation warning and system property to control legacy params (#31597) Tests: Fix timezone conversion in DateTimeUnitTests Enable FIPS140LicenseBootstrapCheck (#32903) Fix InternalAutoDateHistogram reproducible failure (#32723) Remove assertion in testDocStats on deletedDocs counter (#32914) HLRC: Move ML request converters into their own class (#32906) ...

rationull · 2018-08-20T00:52:29Z

@colings86 I created #32979 for removing the deprecated code and TODOs.

) property to control legacy params (#31597) * Scripted metric aggregations: add deprecation warning and system property to control legacy params Scripted metric aggregation params._agg/_aggs are replaced by state/states context variables. By default the old params are still present, and a deprecation warning is emitted when Scripted Metric Aggregations are used. A new system property can be used to disable the legacy params. This functionality will be removed in a future revision. * Fix minor style issue and docs test failure * Disable deprecated params._agg/_aggs in tests and revise tests to use state/states instead * Add integration test covering deprecated scripted metrics aggs params._agg/_aggs access * Disable deprecated params._agg/_aggs in docs integration tests and revise stored scripts to use state/states instead * Revert unnecessary migrations doc change A relevant note should be added in the changes destined for 7.0; this PR is going to be backported to 6.x. * Replace deprecated _agg param bwc integration test with a couple of unit tests * Fix compatibility test after merge * Rename backwards compatibility system property per code review feedback * Tweak deprecation warning text per review feedback buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy server/src/test/java/org/elasticsearch/search/aggregations/metrics/scrip ted/ScriptedMetricAggregatorTests.java /Users/colings86/dev/work/git/elasticsearch/.git/worktrees/elasticsearch -6.x/CHERRY_PICK_HEAD buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy server/src/main/java/org/elasticsearch/script/ScriptedMetricAggContexts. java server/src/main/java/org/elasticsearch/search/aggregations/metrics/scrip ted/InternalScriptedMetric.java server/src/main/java/org/elasticsearch/search/aggregations/metrics/scrip ted/ScriptedMetricAggregatorFactory.java server/src/test/java/org/elasticsearch/search/aggregations/metrics/Scrip tedMetricIT.java server/src/test/java/org/elasticsearch/search/aggregations/metrics/scrip ted/InternalScriptedMetricAggStateV6CompatTests.java server/src/test/java/org/elasticsearch/search/aggregations/metrics/scrip ted/InternalScriptedMetricTests.java server/src/test/java/org/elasticsearch/search/aggregations/metrics/scrip ted/ScriptedMetricAggregatorAggStateV6CompatTests.java server/src/test/java/org/elasticsearch/search/aggregations/metrics/scrip ted/ScriptedMetricAggregatorTests.java

cbuescher · 2019-02-07T14:13:49Z

Removing the "backport pending" label since this has been merged to 6.5 with edb577f

rationull mentioned this pull request Jun 27, 2018

Migrate scripted metric agg scripts to new ScriptContext design and use context variables for agg state #29328

Closed

danielmitterdorfer added >enhancement review labels Jun 27, 2018

danielmitterdorfer added :Analytics/Aggregations Aggregations v7.0.0 labels Jun 27, 2018

rjernst added the v6.4.0 label Jul 5, 2018

Merge branch 'master' into rationull/metric-agg-context-deprecation-a…

c7a6ad1

…nd-disable

colings86 reviewed Jul 12, 2018

View reviewed changes

Fix minor style issue and docs test failure

bc86682

Merge branch 'master' into rationull/metric-agg-context-deprecation-a…

e49f8f7

…nd-disable

colings86 reviewed Jul 16, 2018

View reviewed changes

Merge branch 'master' into rationull/metric-agg-context-deprecation-a…

22f74ed

…nd-disable

Merge branch 'master' into rationull/metric-agg-context-deprecation-a…

087c005

…nd-disable

rjernst approved these changes Aug 16, 2018

View reviewed changes

colings86 approved these changes Aug 17, 2018

View reviewed changes

colings86 merged commit a08127c into elastic:master Aug 17, 2018

colings86 added v6.5.0 and removed v6.4.0 labels Aug 17, 2018

colings86 mentioned this pull request Aug 17, 2018

Backport: Scripted metric aggregations: add deprecation warning and system #32944

Merged

colings86 added the backport pending label Aug 17, 2018

jkakavas mentioned this pull request Aug 17, 2018

[CI] ScriptedMetricIT testInitMapWithParams fails #32959

Closed

rationull mentioned this pull request Aug 17, 2018

Protect ScriptedMetricIT test cases against failures on 0-doc shards (#32959) #32968

Merged

rationull mentioned this pull request Aug 20, 2018

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

Merged

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

cbuescher removed the backport pending label Feb 7, 2019

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

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

Conversation

rationull commented Jun 27, 2018

elasticmachine commented Jun 27, 2018

colings86 commented Jun 28, 2018

rationull commented Jun 28, 2018

colings86 commented Jun 28, 2018

rjernst commented Jun 30, 2018

rjernst commented Jul 3, 2018

rationull commented Jul 4, 2018

rjernst commented Jul 5, 2018

colings86 commented Jul 9, 2018

rationull commented Jul 12, 2018

colings86 left a comment

Choose a reason for hiding this comment

colings86 Jul 12, 2018

Choose a reason for hiding this comment

colings86 commented Jul 12, 2018

rationull commented Jul 12, 2018

rationull commented Jul 13, 2018

colings86 commented Jul 13, 2018

colings86 commented Jul 13, 2018

rationull commented Jul 14, 2018 • edited Loading

rationull commented Jul 15, 2018

colings86 commented Jul 16, 2018

colings86 Jul 16, 2018

Choose a reason for hiding this comment

rjernst Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

rationull Jul 17, 2018

Choose a reason for hiding this comment

rjernst commented Aug 9, 2018

rationull commented Aug 14, 2018

rjernst commented Aug 14, 2018

rjernst commented Aug 15, 2018

rationull commented Aug 16, 2018

rjernst commented Aug 16, 2018

colings86 left a comment

Choose a reason for hiding this comment

colings86 commented Aug 17, 2018

colings86 commented Aug 17, 2018

rationull commented Aug 17, 2018

colings86 commented Aug 17, 2018

rationull commented Aug 20, 2018 • edited Loading

cbuescher commented Feb 7, 2019

rationull commented Jul 14, 2018 •

edited

Loading

rjernst Jul 16, 2018 •

edited

Loading

rationull commented Aug 20, 2018 •

edited

Loading