Re: ClassLoader created by BlobLibraryCacheManager is not using context classloader

[Jan Lukavský]

Hi Till,

the use-case is pretty much simple - I have a REPL shell in groovy, 
which generates classes at runtime. The actual hierarchy is therefore

 system class loader -> application classloader -> repl classloader 
(GroovyClassLoader actually)

now, when a terminal (sink) operation in the shell occurs, I'm able to 
build a jar, which I can submit to remote cluster (in distributed case). 
But - during testing -  I run the code using local flink. There is no 
(legal) way of adding this new runtime generated jar to local flink. As 
I said, I have a hackish solution which works on JDK <= 8, because it 
uses reflection to call addURL on the application classloader (and 
thefore "pretends", that the generated jar was there all the time from 
the JVM startup). This breaks on JDK >= 9. It might be possible to work 
around this somehow, but I think that the reason why LocalEnvironment is 
not having a way to add jars (as in case of RemoteEnvironment) is that 
is assumes, that you actually have all of the on classpath when using 
local runner. I think that this implies that it either has to use 
context classloader (to be able to work on top of any classloading user 
might have), or is wrong and would need be fixed, so that 
LocalEnvironment would accept files to "stage" - which would mean adding 
them to a class loader (probably URLClassLoader with the application 
class loader as parent).

Or, would you see any other option?

Jan


On 9/3/19 2:00 PM, Till Rohrmann wrote:
Hi Jan,

I've talked with Aljoscha and Stephan offline and we concluded that we
would like to avoid the usage of context class loaders if possible. The
reason for this is that using the context class loader can easily mess up
an otherwise clear class loader hierarchy which makes it hard to reason
about and to debug class loader issues.

Given this, I think it would help to better understand the exact problem
you are trying to solve by using the context class loader. Usually the
usage of the context class loader points towards an API deficiency which we
might be able to address differently.

Cheers,
Till

On Mon, Sep 2, 2019 at 11:32 AM Aljoscha Krettek <aljos...@apache.org>
wrote:

I’m not saying we can’t change that code to use the context class loader.
I’m just not sure whether this might break other things.

Best,
Aljoscha

On 2. Sep 2019, at 11:24, Jan Lukavský <je...@seznam.cz> wrote:

Essentially, the class loader of Flink should be present in parent
hierarchy of context class loader. If FlinkUserCodeClassLoader doesn't use
context class loader, then it is actually impossible to use a hierarchy
like this:
  system class loader -> application class loader -> user-defined class
loader (defines some UDFs to be used in flink program)
Flink now uses the application class loader, and so the UDFs fail to
deserialize on local flink, because the user-defined class loader is
bypassed. Moreover, there is no way to add additional classpath elements
for LocalEnvironment (as opposed to RemoteEnvironment). I'm able to hack
this by calling addURL method on the application class loader (which is
terribly hackish), but that works only on JDK <= 8. No sensible workaround
is available for JDK >= 9.
Alternative solution would be to enable adding jars to class loader when
using LocalEnvironment, but that looks a little odd.
Jan

On 9/2/19 11:02 AM, Aljoscha Krettek wrote:
Hi,

I actually don’t know whether that change would be ok.
FlinkUserCodeClassLoader has taken
FlinkUserCodeClassLoader.class.getClassLoader() as the parent ClassLoader
before my change. See:
https://github.com/apache/flink/blob/release-1.2/flink-runtime/src/main/java/org/apache/flink/runtime/execution/librarycache/FlinkUserCodeClassLoader.java
<
https://github.com/apache/flink/blob/release-1.2/flink-runtime/src/main/java/org/apache/flink/runtime/execution/librarycache/FlinkUserCodeClassLoader.java
.
I have the feeling that this might be on purpose because we want to
have the ClassLoader of the Flink Framework components be the parent
ClassLoader, but I could be wrong. Maybe Stephan would be most appropriate
for answering this.
Best,
Aljoscha

On 30. Aug 2019, at 16:28, Till Rohrmann <trohrm...@apache.org> wrote:

Hi Jan,

this looks to me like a bug for which you could create a JIRA and PR
to fix it. Just to make sure, I've pulled in Aljoscha who is the author of
this change to check with him whether we are forgetting something.
Cheers,
Till

On Fri, Aug 30, 2019 at 3:44 PM Jan Lukavský <je...@seznam.cz <mailto:
je...@seznam.cz>> wrote:
Hi,

I have come across an issue with classloading in Flink's MiniCluster.
The issue is that when I run local flink job from a thread, that has a
non-default context classloader (for whatever reason), this classloader
is not taken into account when classloading user defined functions.
This
is due to [1]. Is this behavior intentional, or can I file a JIRA and
use Thread.currentThread.getContextClassLoader() there? I have
validated
that it fixes issues I'm facing.

Jan

[1]

https://github.com/apache/flink/blob/ce557839d762b5f1ec92aa1885fd3d2ae33d0d0b/flink-runtime/src/main/java/org/apache/flink/runtime/execution/librarycache/BlobLibraryCacheManager.java#L280
<
https://github.com/apache/flink/blob/ce557839d762b5f1ec92aa1885fd3d2ae33d0d0b/flink-runtime/src/main/java/org/apache/flink/runtime/execution/librarycache/BlobLibraryCacheManager.java#L280