Make Painless the Default Language #20017
Conversation
| @@ -59,7 +59,7 @@ public ScriptSettings(ScriptEngineRegistry scriptEngineRegistry, ScriptContextRe | |||
| this.scriptLanguageSettings = Collections.unmodifiableList(scriptLanguageSettings); | |||
|
|
|||
| this.defaultScriptLanguageSetting = new Setting<>("script.default_lang", DEFAULT_LANG, setting -> { | |||
| if (!"groovy".equals(setting) && !scriptEngineRegistry.getRegisteredLanguages().containsKey(setting)) { | |||
| if (!"painless".equals(setting) && !scriptEngineRegistry.getRegisteredLanguages().containsKey(setting)) { | |||
There was a problem hiding this comment.
Perhaps use DEFAULT_LANG here instead of hardcoding "painless"?
There was a problem hiding this comment.
Is this first clause even necessary? I don't understand why painless (or groovy before) get's a "free pass" here from existing as a registered engine.
There was a problem hiding this comment.
Good point, that'd be even better
|
@jdconrad I think this definitely needs a big blurb in the migration guide about the switch, with links to the documentation and all that |
|
This is going to break percolators and watches that use scripts but don't specify a |
|
@clintongormley This will be my first time modifying this document. Should I only write a blurb under the migration/migrate_5_0/scripting.asciidoc specifically? Or are there other places this needs to be mentioned as well? |
|
@dakrone @clintongormley I updated the migration doc to include a blurb about the switch. |
| @@ -1,6 +1,54 @@ | |||
| [[breaking_50_scripting]] | |||
| === Script related changes | |||
|
|
|||
| ==== Switched Default Language to Painless | |||
There was a problem hiding this comment.
Mention (for people that might not have a lot of history with ES) that this was migrated from Groovy to Painless
|
Two small comments, otherwise docs LGTM |
|
Sorry for coming late to this, but this feels premature. v5 will be the very first version of Elasticsearch that contains the new scripting language of Painless. To make it default before the community has time to test, start to adopt, etc. seems pre-mature from an external/user perspective. Seeing this happen in a deprecation and then switch to it by default in the next major release after v5 seems much more reasonable. What concerns me (assuming I'm reading between the lines on the implementation) is that once upgraded if a person had scripts being used that did not explicitly declare Since many simple groovy scripts will run fine as painless, this is a dangerous false sense that someone would be OK. I would very much prefer this to be changed back and the switched later on as the community actually has a chance to adopt the language. |
|
@djschny Painless was designed and developed from the start to be a replacement for groovy scripting. The syntax being essentially a subset of groovy was to ease pain of upgrading and allow the vast majority of scripts out there to "just work". Delaying the update to default painless won't help anyone, it only hurts. Either a users script will simply continue working after the upgrade (and see performance gains), or it will not compile, and they need only to specify the language explicitly (which they can already do in all versions of elasticsearch, no need to upgrade).
This is entirely the point of the tedious work by @jdconrad and @rmuir that went into making many many groovy features and syntax available in painless. We have trappy problems like #20097 with groovy performance. Forcing most users that don't know any better (ie just use whatever the default language is) to have degraded performance isn't good. We can help most of those users by changing the default. And it's not dangerous; it is more performant and safer by design. |
|
@djschny folks that really don't wanna take the risk can still go and set |
|
Based upon the conversation, perhaps it makes more sense to have the |
|
Updated the migration doc based on feedback and fixed some more tests. I am for adding the lang as required, but as a separate issue. I don't want this to be held up because of that. If a script works it's transparent to the user, and if not the user is using more advanced features and should be able to figure out what's going on based on the error message and migration docs. There are now several paragraphs in the migration doc on only for this change. |
|
+1 to push. I don't see any actual technical objections, just baseless whining and resistance to change. |
|
This was merged -- I squashed since I had some poor commit messages on the branch. |
|
Thanks to all who reviewed! |
@djschny can you open a new issue for this, I think it's a good idea to discuss |
sure thing, just did on #20122 and tried to add as much explanation/rationale in regards to why I had brought the topic up |
clintongormley
added
:Core/Infra/Scripting
As the title says.
I added the breaking label because there are places where it's possible users are using Groovy features that Painless does not have/support. However, Painless does cover common cases where the user isn't doing something like attempting to log out of a script or other things that should probably not be done there.
Closes #19960