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
File scripts cache key to include language and prevent conflicts #10033
Conversation
} | ||
} | ||
} | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should just build a map for this? I don't like the linear nature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not that we'll ever have thousands of engines registered, but we can do this yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that doesn't matter here really it's just so much simpler to get the stuff from it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, also noticed that we were closing multiple times the same engine pontentially, fixed that too.
af88036
to
4e3a9a1
Compare
Pushed a new commit, can you have a look @s1monw please? |
@@ -194,8 +202,8 @@ public void setClient(Client client) { | |||
} | |||
|
|||
public void close() { | |||
for (ScriptEngineService engineService : scriptEngines.values()) { | |||
engineService.close(); | |||
for (ScriptEngineService scriptEngine : scriptEngines) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you use IOUtils.close()
here please it will close all even if there are exceptions
left one comment other than that LGTM |
@s1monw I made |
for (ScriptEngineService engineService : scriptEngines.values()) { | ||
engineService.close(); | ||
} | ||
IOUtils.closeWhileHandlingException(scriptEngines); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use just close here
left a tiny comment LGTM otherwise |
aba38de
to
7cb709b
Compare
7cb709b
to
2a29e2b
Compare
…licts The file scripts cache key should include the language of the script to prevent conflicts between scripts with same name but different extension (hence lang). Note that script engines can register multiple acronyms that may be used as lang at execution time (e.g. javascript and js are synonyms). We then need to make sure that the same script gets loaded no matter which of the acronyms is used at execution time. The problem didn't exist before this change ad the lang was simply ignored, while now we take it into account. This change has also some positive effect on inline scripts caching. Up until now, the same script referred to with different acronyms would be compiled and cached multiple times, once per acronym. After this change every script gets compiled and cached only once, as we chose internally the acronym used as part of the cache key, no matter which one the user provides. Closes elastic#10033
2a29e2b
to
67a3f31
Compare
…licts The file scripts cache key should include the language of the script to prevent conflicts between scripts with same name but different extension (hence lang). Note that script engines can register multiple acronyms that may be used as lang at execution time (e.g. javascript and js are synonyms). We then need to make sure that the same script gets loaded no matter which of the acronyms is used at execution time. The problem didn't exist before this change ad the lang was simply ignored, while now we take it into account. This change has also some positive effect on inline scripts caching. Up until now, the same script referred to with different acronyms would be compiled and cached multiple times, once per acronym. After this change every script gets compiled and cached only once, as we chose internally the acronym used as part of the cache key, no matter which one the user provides. Closes elastic#10033
…licts The file scripts cache key should include the language of the script to prevent conflicts between scripts with same name but different extension (hence lang). Note that script engines can register multiple acronyms that may be used as lang at execution time (e.g. javascript and js are synonyms). We then need to make sure that the same script gets loaded no matter which of the acronyms is used at execution time. The problem didn't exist before this change ad the lang was simply ignored, while now we take it into account. This change has also some positive effect on inline scripts caching. Up until now, the same script referred to with different acronyms would be compiled and cached multiple times, once per acronym. After this change every script gets compiled and cached only once, as we chose internally the acronym used as part of the cache key, no matter which one the user provides. Closes elastic#10033
…licts The file scripts cache key should include the language of the script to prevent conflicts between scripts with same name but different extension (hence lang). Note that script engines can register multiple acronyms that may be used as lang at execution time (e.g. javascript and js are synonyms). We then need to make sure that the same script gets loaded no matter which of the acronyms is used at execution time. The problem didn't exist before this change ad the lang was simply ignored, while now we take it into account. This change has also some positive effect on inline scripts caching. Up until now, the same script referred to with different acronyms would be compiled and cached multiple times, once per acronym. After this change every script gets compiled and cached only once, as we chose internally the acronym used as part of the cache key, no matter which one the user provides. Closes elastic#10033
The file scripts cache key should include the language of the script to prevent conflicts between scripts with same name but different extension (hence lang). Note that script engines can register multiple acronyms that may be used as lang at execution time (e.g. javascript and js are synonyms). We then need to make sure that the same script gets loaded no matter which of the acronyms is used at execution time. This problem didn't exist before this change as the lang was simply ignored, while now we take it into account.
This change has also some positive effect on inline scripts caching. Up until now, the same script referred to with different acronyms would be compiled and cached multiple times, once per acronym. After this change every script gets compiled and cached only once, as we chose internally the acronym used as part of the cache key, no matter which one the user provides.
Also made sure that the same engine is closed only once.