DRILL-7317: Close ClassLoaders used for udf jars uploading when closing FunctionImplementationRegistry #1822
Conversation
| * This is write operation, so one user at a time can call perform such action, | ||
| * others will wait till first user completes his action. | ||
| */ | ||
| public void removeAllJars() { |
There was a problem hiding this comment.
To improve performance please consider adding close method that will just close appropriate classloaders and then nullify map holders.
There was a problem hiding this comment.
Good idea!
I have made this change, could you please review it again?
|
The latest change caused some tests failures, I'll inform when they are resolved... |
| } | ||
| }); | ||
|
|
||
| jars.clear(); |
There was a problem hiding this comment.
Giving a second thought we might consider not clearing the maps (better decision would be to nullify them but I assume we won't to remove final flag), then I suggest we leave them as is, since we are closing the Drillbit anyway.
There was a problem hiding this comment.
Agree, reverted this change.
| /** | ||
| * Produces search of {@link DrillFuncHolder} which corresponds to specified {@code String functionName} | ||
| * with signature from {@code Queue<String> functionSignatures}, | ||
| * closes its class loader if {@link DrillFuncHolder} is found and returns true. Otherwise faalse is returned. |
There was a problem hiding this comment.
| * closes its class loader if {@link DrillFuncHolder} is found and returns true. Otherwise faalse is returned. | |
| * closes its class loader if {@link DrillFuncHolder} is found and returns true. Otherwise false is returned. |
| Map<String, DrillFuncHolder> functionHolders = functions.get(functionName); | ||
| Iterator<String> signaturesIterator = functionSignatures.iterator(); | ||
| DrillFuncHolder drillFuncHolder = null; | ||
| while (signaturesIterator.hasNext() && drillFuncHolder == null) { |
There was a problem hiding this comment.
Consider using streams findAny.
There was a problem hiding this comment.
Thanks, it looks better with findAny.
| * | ||
| * @param functionName name of the function | ||
| * @param functionSignatures function signatures | ||
| * @return {@code true} if {@link DrillFuncHolder} is found and its class loader is closed |
There was a problem hiding this comment.
Not exactly, you return true if even there was a failure when closing class loader, more correct would be to describe as return true if func holder was found and attempted to close class loader disregarding the result.
There was a problem hiding this comment.
Thanks, replaced it with the sentence you have proposed.
| } | ||
| } | ||
| // closes class loader only one time | ||
| isClosed = !isClosed && closeClassLoader(function, functionSignatures); |
There was a problem hiding this comment.
There is only once class loader per jar, we can consider using findAny as I have suggested below.
There was a problem hiding this comment.
That's correct that there is only one classloader per jar, but except closing it, we should also remove function signatures from functions map, so we need to iterate further.
With the current code, closeClassLoader method is called only once per jar.
There was a problem hiding this comment.
@arina-ielchiieva, thanks for the review. I have addressed CR comments and tests failures.
Changes in the previous commit exposed interesting issue - when several drillbits are run within the same JVM, DrillMergeProjectRule caches its instance and FunctionImplementationRegistry, so all further DrillBits use cached version, but their own may have or may have not some specific functions.
It is important for the case when a user registers functions with the complex output so in this case DrillMergeProjectRule may merge project which cannot be merged.
The solution I propose is to remove caching of DrillMergeProjectRule.
It is added in a separate commit.
| } | ||
| }); | ||
|
|
||
| jars.clear(); |
There was a problem hiding this comment.
Agree, reverted this change.
| /** | ||
| * Produces search of {@link DrillFuncHolder} which corresponds to specified {@code String functionName} | ||
| * with signature from {@code Queue<String> functionSignatures}, | ||
| * closes its class loader if {@link DrillFuncHolder} is found and returns true. Otherwise faalse is returned. |
| * | ||
| * @param functionName name of the function | ||
| * @param functionSignatures function signatures | ||
| * @return {@code true} if {@link DrillFuncHolder} is found and its class loader is closed |
There was a problem hiding this comment.
Thanks, replaced it with the sentence you have proposed.
| Map<String, DrillFuncHolder> functionHolders = functions.get(functionName); | ||
| Iterator<String> signaturesIterator = functionSignatures.iterator(); | ||
| DrillFuncHolder drillFuncHolder = null; | ||
| while (signaturesIterator.hasNext() && drillFuncHolder == null) { |
There was a problem hiding this comment.
Thanks, it looks better with findAny.
| } | ||
| } | ||
| // closes class loader only one time | ||
| isClosed = !isClosed && closeClassLoader(function, functionSignatures); |
There was a problem hiding this comment.
That's correct that there is only one classloader per jar, but except closing it, we should also remove function signatures from functions map, so we need to iterate further.
With the current code, closeClassLoader method is called only once per jar.
|
+1, please squash the commits. |
vvysotskyi
force-pushed
the
DRILL-7317
branch
2 times, most recently
from
4e35df6
to
538ca5e
Compare
…ng FunctionImplementationRegistry - Fix issue with caching DrillMergeProjectRule and FunctionImplementationRegistry when different drillbits are started within the same JVM
Please see DRILL-7317 for problem description.