pnowojski commented on a change in pull request #9271: 
[FLINK-13384][1.9][runtime] Fix back pressure sampling for SourceStreamTask
URL: https://github.com/apache/flink/pull/9271#discussion_r309646392
 
 

 ##########
 File path: 
flink-runtime/src/main/java/org/apache/flink/runtime/taskmanager/Task.java
 ##########
 @@ -593,6 +593,10 @@ else if (current == ExecutionState.CANCELING) {
                        LOG.info("Loading JAR files for task {}.", this);
 
                        userCodeClassLoader = createUserCodeClassloader();
+
+                       // make sure the user code classloader is accessible 
thread-locally
+                       
executingThread.setContextClassLoader(userCodeClassLoader);
 
 Review comment:
   Maybe moving this just before
   ```
   invokable = loadAndInstantiateInvokable(userCodeClassLoader, 
nameOfInvokableClass, env);
   ```
   would be more clean and safe? And maybe add some comment like
   ```
   // We are setting the correct context class loader for the whole interaction 
with the `invokable`
   ```
   ? 
   
   The contract that we would introduce here, is that user code should be 
executed with the `userCodeClassLoader`. Before constructing the invokable, we 
are not touching any user code, so there should be no point (and could 
potentially do some harm) in setting context class loader so early.
   
   Arguably this is a separate bug fix that just wasn't discovered before.
   
   Also technically speaking it would be better to reset the context class 
loader when we are done interacting with the `invokable`, however this might be 
risky change, so I wouldn't do it now, but sometime in the future.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
[email protected]


With regards,
Apache Git Services

Reply via email to