nateab commented on code in PR #27579:
URL: https://github.com/apache/flink/pull/27579#discussion_r3693893639
##########
flink-runtime/src/main/java/org/apache/flink/runtime/execution/librarycache/BlobLibraryCacheManager.java:
##########
@@ -238,25 +238,41 @@ private UserCodeClassLoader getOrResolveClassLoader(
verifyIsNotReleased();
if (resolvedClassLoader == null) {
- boolean systemClassLoader =
- wrapsSystemClassLoader && libraries.isEmpty() &&
classPaths.isEmpty();
- resolvedClassLoader =
- new ResolvedClassLoader(
- systemClassLoader
- ?
ClassLoader.getSystemClassLoader()
- : createUserCodeClassLoader(
- jobId, applicationId,
libraries, classPaths),
- libraries,
- classPaths,
- systemClassLoader);
+ resolvedClassLoader = createResolvedClassLoader(libraries,
classPaths);
} else {
- resolvedClassLoader.verifyClassLoader(libraries,
classPaths);
+ try {
+ resolvedClassLoader.verifyClassLoader(libraries,
classPaths);
+ } catch (IllegalStateException e) {
+ LOG.warn(
+ "Library cache entry for job {} has a
classloader resolved with different "
+ + "library BLOBs than requested. This
can happen during JobManager "
+ + "failover. Re-creating the
classloader with the new blob keys.",
+ jobId,
+ e);
+ resolvedClassLoader =
createResolvedClassLoader(libraries, classPaths);
Review Comment:
You're right it isn't OK to leave unreleased: it leaks the URLClassLoader,
and releaseHooks is per-ResolvedClassLoader and only runs from
releaseClassLoader(), so hooks on the replaced instance would never run at all.
One wrinkle on the line you linked, releaseClassLoader() is also called from
BlobLibraryCacheManager#shutdown(), not just the refcount-zero path.
Digging in surfaced something worse: resolvedClassLoader is a single field
on LibraryCacheEntry, shared by every lease for the job. Replacing it
invalidates what other leases already resolved, so during a failover old and
new keys would alternate and re-create on every call, leaking each time.
So re-creation is gone, verifyClassLoader compares content hashes, nothing
is replaced, and there's no stale instance to track. Added
classLoaderIsSharedAcrossLeasesWhenBlobKeysDifferOnlyInRandomComponent for the
multi-lease case.
--
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]
