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

Clear the GroovyClassLoader cache before compiling #8062

Merged
merged 1 commit into from Oct 14, 2014

Conversation

Projects
None yet
5 participants
@dakrone
Copy link
Member

dakrone commented Oct 13, 2014

Since we don't use the cache, it's okay to clear it entirely if needed,
Elasticsearch maintains its own cache for compiled scripts.

Fixes #7658
Fixes #8073

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Oct 13, 2014

++ I think this is a a bug really no? should it go to 1.3.5 and 1.4 too? LGTM

@s1monw s1monw removed the review label Oct 13, 2014

@dakrone

This comment has been minimized.

Copy link
Member Author

dakrone commented Oct 13, 2014

Refactored this after talking with @kimchy, this is now implemented as a listener where the classloader is only cleared when scripts are removed from the cache.

I tested this with the script from #7658 and I was able to send/compile 10,000 unique scripts on both Java 7 and 8 without running into PermGen issues.

@s1monw

View changes

src/main/java/org/elasticsearch/script/ScriptService.java Outdated
if (logger.isDebugEnabled()) {
logger.debug("notifying script services of script removal due to: [{}]", notification.getCause());
}
for (ScriptEngineService service : scriptEngines.values()) {

This comment has been minimized.

Copy link
@s1monw

s1monw Oct 14, 2014

Contributor

maybe catch exceptions here and put them into an arraylist, you can then just use ExceptionHelper to rerthrow and surpress....it has a utility for this

@s1monw

View changes

src/main/java/org/elasticsearch/script/groovy/GroovyScriptEngineService.java Outdated
@@ -90,6 +89,16 @@ public void close() {
}

@Override
public void scriptRemoved(@Nullable CompiledScript script) {
// script could be null, meaning the script has already been garbage collected
if (script == null || script.lang().equals("groovy")) {

This comment has been minimized.

Copy link
@s1monw

s1monw Oct 14, 2014

Contributor

for safety can you do "groovy".equals(script.lang()) instead

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Oct 14, 2014

left minor comments - LGTM

@dakrone dakrone force-pushed the dakrone:clear-groovy-permgen branch 2 times, most recently to 2c6d31d Oct 14, 2014

Clear the GroovyClassLoader cache before compiling
Since we don't use the cache, it's okay to clear it entirely if needed,
Elasticsearch maintains its own cache for compiled scripts.

Adds loader.clearCache() into a listener, the listener is called when a
script is removed from the Guava cache.

This also lowers the amount of cached scripts to 100, since 500 is
around the limit some users have run into before hitting an out of
memory error in permgem.

Fixes #7658

@dakrone dakrone merged commit 2c6d31d into elastic:master Oct 14, 2014

@dadoonet

This comment has been minimized.

Copy link
Member

dadoonet commented Oct 15, 2014

This one breaks script plugins for elasticsearch 1.3.5, 1.4.0 and above.

A new method in ScriptEngineService broke the contract:

void scriptRemoved(@Nullable CompiledScript script);

dadoonet added a commit to elastic/elasticsearch-lang-javascript that referenced this pull request Oct 15, 2014

Implement new method `scriptRemoved(CompiledScript)` in `ScriptEngine…
…Service`

This [PR](elastic/elasticsearch#8062) broke ScriptEngineService by adding a new method `scriptRemoved(CompiledScript)`.

Closes #29.

(cherry picked from commit 1fd05bc)

dadoonet added a commit to elastic/elasticsearch-lang-javascript that referenced this pull request Oct 15, 2014

Implement new method `scriptRemoved(CompiledScript)` in `ScriptEngine…
…Service`

This [PR](elastic/elasticsearch#8062) broke ScriptEngineService by adding a new method `scriptRemoved(CompiledScript)`.

Closes #29.

(cherry picked from commit 1fd05bc)

dadoonet added a commit to elastic/elasticsearch-lang-javascript that referenced this pull request Oct 15, 2014

Implement new method `scriptRemoved(CompiledScript)` in `ScriptEngine…
…Service`

This [PR](elastic/elasticsearch#8062) broke ScriptEngineService by adding a new method `scriptRemoved(CompiledScript)`.

Closes #29.

dadoonet added a commit to elastic/elasticsearch-lang-javascript that referenced this pull request Oct 15, 2014

Implement new method `scriptRemoved(CompiledScript)` in `ScriptEngine…
…Service`

This [PR](elastic/elasticsearch#8062) broke ScriptEngineService by adding a new method `scriptRemoved(CompiledScript)`.

Closes #29.

(cherry picked from commit 1fd05bc)

dadoonet added a commit to elastic/elasticsearch-lang-mvel that referenced this pull request Oct 15, 2014

Implement new method `scriptRemoved(CompiledScript)` in `ScriptEngine…
…Service`

This [PR](elastic/elasticsearch#8062) broke ScriptEngineService by adding a new method `scriptRemoved(CompiledScript)`.

Closes #6.

(cherry picked from commit 3985cec)

dadoonet added a commit to elastic/elasticsearch-lang-mvel that referenced this pull request Oct 15, 2014

Implement new method `scriptRemoved(CompiledScript)` in `ScriptEngine…
…Service`

This [PR](elastic/elasticsearch#8062) broke ScriptEngineService by adding a new method `scriptRemoved(CompiledScript)`.

Closes #6.

(cherry picked from commit 3985cec)

dadoonet added a commit to elastic/elasticsearch-lang-mvel that referenced this pull request Oct 15, 2014

Implement new method `scriptRemoved(CompiledScript)` in `ScriptEngine…
…Service`

This [PR](elastic/elasticsearch#8062) broke ScriptEngineService by adding a new method `scriptRemoved(CompiledScript)`.

Closes #6.

dadoonet added a commit to elastic/elasticsearch-lang-python that referenced this pull request Oct 15, 2014

Implement new method `scriptRemoved(CompiledScript)` in `ScriptEngine…
…Service`

This [PR](elastic/elasticsearch#8062) broke ScriptEngineService by adding a new method `scriptRemoved(CompiledScript`.

Closes #23.

(cherry picked from commit 8afd168)

dadoonet added a commit to elastic/elasticsearch-lang-python that referenced this pull request Oct 15, 2014

Implement new method `scriptRemoved(CompiledScript)` in `ScriptEngine…
…Service`

This [PR](elastic/elasticsearch#8062) broke ScriptEngineService by adding a new method `scriptRemoved(CompiledScript`.

Closes #23.

(cherry picked from commit 8afd168)

dadoonet added a commit to elastic/elasticsearch-lang-python that referenced this pull request Oct 15, 2014

Implement new method `scriptRemoved(CompiledScript)` in `ScriptEngine…
…Service`

This [PR](elastic/elasticsearch#8062) broke ScriptEngineService by adding a new method `scriptRemoved(CompiledScript)`.

Closes #23.

(cherry picked from commit 8afd168)

dadoonet added a commit to elastic/elasticsearch-lang-python that referenced this pull request Oct 15, 2014

Implement new method `scriptRemoved(CompiledScript)` in `ScriptEngine…
…Service`

This [PR](elastic/elasticsearch#8062) broke ScriptEngineService by adding a new method `scriptRemoved(CompiledScript)`.

Closes #23.

@jpountz jpountz removed the review label Oct 21, 2014

@clintongormley clintongormley changed the title Clear the GroovyClassLoader cache before compiling Scripting: Clear the GroovyClassLoader cache before compiling Nov 3, 2014

@dakrone dakrone deleted the dakrone:clear-groovy-permgen branch Nov 11, 2014

@clintongormley clintongormley changed the title Scripting: Clear the GroovyClassLoader cache before compiling Clear the GroovyClassLoader cache before compiling Jun 8, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.