[FLINK-6606] Create checkpoint hook with user classloader

[EronWright]

• wrap calls to MasterTriggerRestoreHook (and its factory) such that the user classloader is applied

[rmetzger]

I haven't worked on this part of the code before, but I believe this code is +1 to merge.

[tillrohrmann]

Thanks for your contribution @EronWright. I'm not quite sure whether I understand which problem we are trying to solve here.

I think by deserializing the MasterTriggerRestoreHook.Factories with the user code class loader in ExecutionGraphBuilder.java:253, we support user code hooks. Given that the Factory is a user defined class, then it should get the user code class loader set as its ClassLoader. Thus, it should also be able to load a user defined MasterTriggerRestoreHook class. And the latter can only be a user defined class if the factory is user defined.

But I might be overlooking something here. Maybe you can give me some more details about the PR.

[EronWright]

@tillrohrmann the issue is with code that uses the thread's context classloader (TCCL), that may execute in Factory.create or later in the hook methods. For example, the Pravega connector uses grpc, which uses the TCCL during initialization (see ManagedChannelProvider).

Looking at other areas in Flink where usercode is called, I see that the TCCL is consistently set. For example, see InputFormatVertex, ExecutionJobVertex, and Task.

[rmetzger]

Till and I had an offline discussion about this.
What you can do in your user code to set the TCCL properly is the following:

Thread.currentThread().setContextClassLoader(this.getClass().getClassLoader());

this points in user code to a class loaded through the user code classlaoder. So this.getClass().getClassLoader() is the CL you need to set for the thread.

[EronWright]

That's the sort of workaround that Android apps are forced to use. Why not continue the practice of setting the TCCL in transitions from system to user code?

[rmetzger]

Lets see what Till says.
In my opinion, both approaches are fine. What you are proposing makes things on Flink's side a bit more complicated, but the behavior is consistent with other places where user code is executed.

Till's proposal (so get the CL via the class) keeps our code straightforward, but users have to know it.

[tillrohrmann]

I agree with @EronWright that it would be nice to offer all user code the a similar environment. This will become especially important if a MasterTriggerRestoreHook which is contained in the system class loader tries to load user code classes dynamically.

I'm wondering whether to follow the approach of the InputFormatVertex and ExecutionJobVertex to set the TCCL explicitly when calling the hooks. This could happen in MasterHooks#triggerMasterHooks for example. The advantage would be that we don't have to reset the TCCL for each hook. On the other hand, it harbours danger that we will forget to set the TCCL when calling the hooks from somewhere else in the future.

[tillrohrmann]

Maybe we can move the setContextClassLoader into the try body. Just to be on the safe side that whenever something happens we will reset the original class loader.

[EronWright]

I disagee with that approach due to lack of symmetry. The try..finally idiom is typically written as:

open();
try { ... } finally { close(); }

In other words, the close statement is needed only if the open statement completes.

[tillrohrmann]

True.

[EronWright]

Originally I pursued the path suggested by @tillrohrmann of setting the TCCL at the sites where the hook is invoked, but found that the classloader wasn't readily available. I found it more expedient to create a wrapper that eagerly captured the classloader. This also improved testability.

[tillrohrmann]

Yes that is true. Thanks for your work and the clarification @EronWright. Merging this PR.