github-actions[bot] commented on code in PR #63897:
URL: https://github.com/apache/doris/pull/63897#discussion_r3323025724
##########
fe/be-java-extensions/java-udf/src/main/java/org/apache/doris/udf/BaseExecutor.java:
##########
@@ -143,7 +143,9 @@ public UdfClassCache getClassCache(String jarPath, String
signature, long expira
classLoader = cache.classLoader;
}
}
- if (cache == null) {
+ // Rebuild if cache is missing or if the cached classLoader was closed
by a concurrent
+ // ExpiringMap expiration (cache != null but classLoader == null).
+ if (cache == null || cache.classLoader == null) {
Review Comment:
`cache.classLoader == null` is a valid cached state when `jarPath` is empty
and the UDF is loaded from `custom_lib` via the system classloader;
`UdfClassCache` documents that null is intentional in that case. With this
condition, every static-load invocation of such a UDF treats the existing cache
as invalid, rebuilds the reflection metadata, and overwrites the map instead of
reusing the static cache. Since this PR removes ExpiringMap eviction, null no
longer reliably means an expired/closed cached loader here; please distinguish
the system-loader case or only treat an actual cache miss as requiring rebuild.
##########
fe/be-java-extensions/java-common/src/main/java/org/apache/doris/common/classloader/ScannerLoader.java:
##########
@@ -116,15 +125,46 @@ public static UdfClassCache getUdfClassLoader(String
functionSignature) {
return udfLoadedClasses.get(functionSignature);
}
- public static synchronized void cacheClassLoader(String functionSignature,
UdfClassCache classCache,
+ /**
+ * Cache the UDF class metadata for the given function signature.
+ * The {@code expirationTime} parameter is kept for backward compatibility
with the
+ * existing call sites and DDL property {@code expiration_time}, but is no
longer used:
+ * cached entries are not evicted by time. Removal happens only via
+ * {@link #cleanUdfClassLoader(String)} on DROP FUNCTION.
+ */
+ public static void cacheClassLoader(String functionSignature,
UdfClassCache classCache,
long expirationTime) {
- LOG.info("Cache UDF for: {}", functionSignature);
Review Comment:
This replacement is not safe for concurrent first use of the same static
UDF. Two executor threads can both observe a miss in
`BaseExecutor.getClassCache()`, build separate `UdfClassCache` instances, and
then call `cacheClassLoader`; the second `put` closes the first cache's
`URLClassLoader` here while the first executor has already kept that cache and
may still lazily resolve dependent classes. That reintroduces the same
live-classloader close / `NoClassDefFoundError` failure mode the PR is trying
to remove. Please make construction/insertion atomic, for example with
`computeIfAbsent`/`putIfAbsent`, and only close the newly-created unused cache
rather than a cache that another executor may already be using.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
