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

Consistently name Groovy scripts with the same content #12296

Merged
merged 1 commit into from Jul 16, 2015

Conversation

dakrone
Copy link
Member

@dakrone dakrone commented Jul 16, 2015

When adding a script to the Groovy classloader, the script name is used
as the class identifier in the classloader. This means that in order not
to break JVM Classloader convention, that script must always be
available by that name. As a result, modifying a script with the same
content over and over causes it to be loaded with a different name (due
to the incrementing integer).

This is particularly bad when something like chef or puppet replaces the
on-disk script file with the same content over and over every time a
machine is converged.

This change makes the script name the SHA1 hash of the script itself,
meaning that replacing a script with the same text will use the same
script name.

Resolves #12212

As a test for this, I did the following:

  • Configure the resource watcher to check for new scripts more frequently (every 100ms)
watcher.interval.low: 100ms
watcher.interval.medium: 100ms
watcher.interval.high: 100ms
resource.reload.interval.low: 100ms
resource.reload.interval.medium: 100ms
resource.reload.interval.high: 100ms
  • Start ES with a 1.7 JVM (since 1.8 removed permgen)
  • Run a script that constantly updated a script file with the same content over and over, causing it to be re-compiled by Elasticsearch:
while true; do
  echo "ctx._source.counter += 1" > config/scripts/myscript.groovy
  echo "ctx._source.counter += 2" > config/scripts/myscript.groovy
done

Without this change, permgen grows linearly, then the ES node hits OOME in permgen:

screenshot from 2015-07-16 12-42-15

And with this change, the classes are able to be unloaded, because they share the same class name (SHA1 of the content):

screenshot from 2015-07-16 12-34-20

@jasontedor
Copy link
Member

Nice. LGTM.

When adding a script to the Groovy classloader, the script name is used
as the class identifier in the classloader. This means that in order not
to break JVM Classloader convention, that script must always be
available by that name. As a result, modifying a script with the same
content over and over causes it to be loaded with a different name (due
to the incrementing integer).

This is particularly bad when something like chef or puppet replaces the
on-disk script file with the same content over and over every time a
machine is converged.

This change makes the script name the SHA1 hash of the script itself,
meaning that replacing a script with the same text will use the same
script name.

Resolves elastic#12212
@dakrone dakrone force-pushed the groovy-permgen-script-name branch from 1e2eea2 to d902012 Compare July 16, 2015 21:45
@dakrone dakrone merged commit d902012 into elastic:master Jul 16, 2015
@clintongormley clintongormley added the :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache label Jul 17, 2015
@dakrone dakrone deleted the groovy-permgen-script-name branch May 13, 2016 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v1.7.1 v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants