[GitHub] flink issue #4980: [FLINK-8005] [runtime] Set user code class loader before ...

2017-11-10 Thread aljoscha 
Github user aljoscha commented on the issue:

https://github.com/apache/flink/pull/4980
  
Thanks again for this fix! 👍 

Could you please close if GitHub doesn't auto-close?


---

[GitHub] flink issue #4980: [FLINK-8005] [runtime] Set user code class loader before ...

2017-11-09 Thread kl0u 
Github user kl0u commented on the issue:

https://github.com/apache/flink/pull/4980
  
I agree! +1 to merge as soon as Travis gives us the green light.


---

[GitHub] flink issue #4980: [FLINK-8005] [runtime] Set user code class loader before ...

2017-11-09 Thread aljoscha 
Github user aljoscha commented on the issue:

https://github.com/apache/flink/pull/4980
  
thanks, I think this is excellent now. 👌 

I'll merge as soon as travis is green.


---

[GitHub] flink issue #4980: [FLINK-8005] [runtime] Set user code class loader before ...

2017-11-09 Thread aljoscha 
Github user aljoscha commented on the issue:

https://github.com/apache/flink/pull/4980
  
Yes, but I think this is making an assumption about the internal 
implementation. If someone changes that the test could break/not test the right 
thing anymore.


---

[GitHub] flink issue #4980: [FLINK-8005] [runtime] Set user code class loader before ...

2017-11-09 Thread GJL 
Github user GJL commented on the issue:

https://github.com/apache/flink/pull/4980
  
There is only one thread dispatching the calls:
```
executor = Executors.newSingleThreadExecutor(
new 
DispatcherThreadFactory(TASK_THREADS_GROUP, "Async calls on " + 
taskNameWithSubtask));
this.asyncCallDispatcher = executor;
```

The tasks cannot overtake each other. I could make the test more strict and 
wait additionally on `triggerLatch` in case somebody decides to have multiple 
threads.


---

[GitHub] flink issue #4980: [FLINK-8005] [runtime] Set user code class loader before ...

2017-11-09 Thread aljoscha 
Github user aljoscha commented on the issue:

https://github.com/apache/flink/pull/4980
  
I think waiting on the stop latch might not be enough (in 100 % of cases) 
because the other two calls are also asynchronous.


---

[GitHub] flink issue #4980: [FLINK-8005] [runtime] Set user code class loader before ...

2017-11-09 Thread GJL 
Github user GJL commented on the issue:

https://github.com/apache/flink/pull/4980
  
I addressed the comments. Let's wait for Travis and let me know if 
something else needs to be changed. 

@aljoscha  @kl0u 


---

[GitHub] flink issue #4980: [FLINK-8005] [runtime] Set user code class loader before ...

2017-11-08 Thread aljoscha 
Github user aljoscha commented on the issue:

https://github.com/apache/flink/pull/4980
  
These changes look good! 👍 

I'll wait for travis and then merge.


---