zentol commented on code in PR #22718:
URL: https://github.com/apache/flink/pull/22718#discussion_r1232139518
##########
flink-runtime/src/main/java/org/apache/flink/runtime/execution/librarycache/BlobLibraryCacheManager.java:
##########
@@ -375,12 +389,15 @@ private static final class ResolvedClassLoader implements
UserCodeClassLoader {
*/
private final Set<String> classPaths;
+ private final boolean systemClassLoader;
+
private final Map<String, Runnable> releaseHooks;
private ResolvedClassLoader(
- URLClassLoader classLoader,
+ ClassLoader classLoader,
Collection<PermanentBlobKey> requiredLibraries,
- Collection<URL> requiredClassPaths) {
+ Collection<URL> requiredClassPaths,
+ boolean systemClassLoader) {
Review Comment:
```suggestion
boolean wrapsSystemClassLoader) {
```
##########
flink-runtime/src/test/java/org/apache/flink/runtime/execution/librarycache/BlobLibraryCacheManagerTest.java:
##########
@@ -148,10 +159,10 @@ public void testLibraryCacheManagerDifferentJobsCleanup()
throws Exception {
assertEquals(2, libCache.getNumberOfManagedJobs());
assertEquals(1, libCache.getNumberOfReferenceHolders(jobId1));
assertEquals(1, libCache.getNumberOfReferenceHolders(jobId2));
- assertEquals(2, checkFilesExist(jobId1, keys1, cache, true));
+ assertEquals(2, checkFilesExist(jobId1, keys1, cache, false));
Review Comment:
shouldn't this be `!useSystemClassLoader`?
##########
flink-runtime/src/main/java/org/apache/flink/runtime/execution/librarycache/LibraryCacheManager.java:
##########
@@ -43,9 +43,11 @@ public interface LibraryCacheManager {
* job will be valid as long as there exists a valid lease for this job.
*
* @param jobId jobId for which to register a new class loader lease
+ * @param useSystemClassLoader use system class loader if the jars or
classpaths of job are
+ * empty
* @return a new class loader lease for the given job
*/
- ClassLoaderLease registerClassLoaderLease(JobID jobId);
+ ClassLoaderLease registerClassLoaderLease(JobID jobId, boolean
useSystemClassLoader);
Review Comment:
hmm. Shouldn't this be controlled per LibraryCacheManager _instance_ instead
of per call, and thus be set in the manager constructor?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
