Skip to content

Scripting: Deprecate scripts.max_compilation_per_minute setting#26402

Merged
spinscale merged 5 commits intoelastic:5.6from
spinscale:1708-deprecate-script-compilations-per-minute-setting
Aug 29, 2017
Merged

Scripting: Deprecate scripts.max_compilation_per_minute setting#26402
spinscale merged 5 commits intoelastic:5.6from
spinscale:1708-deprecate-script-compilations-per-minute-setting

Conversation

@spinscale
Copy link
Copy Markdown
Contributor

This setting will be replaced with script.max_compilation_rate in
Elasticsearch 6.0, thus it can be marked as deprecated in 5.6.

This setting will be replaced with script.max_compilation_rate in
Elasticsearch 6.0, thus it can be marked as deprecated in 5.6.
@spinscale spinscale added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache review v5.6.0 labels Aug 28, 2017
@jasontedor
Copy link
Copy Markdown
Member

I'm surprised the changes aren't larger than this? We set the setting in internal test cluster and REST tests, I expect un-acknowledged warnings to fail the build?

Setting.intSetting("script.max_size_in_bytes", 65535, Property.NodeScope);
public static final Setting<Integer> SCRIPT_MAX_COMPILATIONS_PER_MINUTE =
Setting.intSetting("script.max_compilations_per_minute", 15, 0, Property.Dynamic, Property.NodeScope);
Setting.intSetting("script.max_compilations_per_minute", 15, 0, Property.Dynamic, Property.NodeScope, Property.Deprecated);
Copy link
Copy Markdown
Member

@dakrone dakrone Aug 28, 2017

Choose a reason for hiding this comment

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

I don't feel like we should deprecate this unless there's an actual replacement. It feels strange to say "don't use this setting, but you can't replace it until 6.0".

I think we should port the max_compilation_rate changes to 5.6 so that a user can swap the settings from deprecated to non-deprecated without upgrading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We do this sometimes (e.g., default paths), it's okay.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We discussed this and decided documenting it is fine.

Copy link
Copy Markdown
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

LGTM.

@spinscale spinscale merged commit 4fc899d into elastic:5.6 Aug 29, 2017
spinscale added a commit that referenced this pull request Aug 29, 2017
…ng (#26402)"

This reverts commit 4fc899d.

The order of tests in the REST tests decides about failures, this
needs to be fixed first.
@spinscale
Copy link
Copy Markdown
Contributor Author

I reverted this due to test failures, that are dependent on the order of the tests run in the docs folder. Needs more investigation first.

jasontedor added a commit that referenced this pull request Aug 29, 2017
* 5.6:
  Adapt Netty 3 to avoid blocking on channel close
  Avoid blocking on channel close on network thread (#25521)
  Scripting: Deprecate scripts.max_compilation_per_minute setting (#26420)
  Revert "[Docs] Update Java Low-Level documentation to reflect shaded deps (#25882)"
  [DOCS] Updates 5.6.0 release notes
  [DOCS] Updates 5.5.3 release notes
  Revert "Scripting: Deprecate scripts.max_compilation_per_minute setting (#26402)"
  Scripting: Deprecate scripts.max_compilation_per_minute setting (#26402)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blocker :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >deprecation v5.6.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants