github-actions[bot] commented on code in PR #67046:
URL: https://github.com/apache/doris/pull/67046#discussion_r3862422769
##########
fe/be-java-extensions/java-common/src/main/java/org/apache/doris/common/classloader/ScannerLoader.java:
##########
@@ -126,59 +128,74 @@ public void loadAllScannerJars() {
LOG.info("Finished loading scanner JARs");
}
- public static UdfClassCache getUdfClassLoader(String functionSignature) {
- return udfLoadedClasses.get(functionSignature);
+ private static class UdfClassCacheEntry {
+ private final String functionSignature;
+ private final UdfClassCache classCache;
+
+ UdfClassCacheEntry(String functionSignature, UdfClassCache classCache)
{
+ this.functionSignature = functionSignature;
+ this.classCache = classCache;
+ }
+ }
+
+ public static UdfClassCache getUdfClassLoader(long functionId) {
+ UdfClassCacheEntry entry = udfLoadedClasses.get(functionId);
+ return entry == null ? null : entry.classCache;
}
/**
- * Cache the UDF class metadata for the given function signature.
+ * Cache the UDF class metadata for the given catalog function id.
*
- * <p>Insertion is atomic via {@link Map#putIfAbsent}: if another executor
thread has
- * already published a cache entry for {@code functionSignature}, the
{@code classCache}
+ * <p>Insertion is atomic via {@link Map#putIfAbsent}: if another executor
+ * thread has already published a cache entry for {@code functionId}, the
{@code classCache}
* argument is treated as a redundant build and closed here (it has not
yet been handed
* to any executor, so closing its URLClassLoader is safe). The
already-published entry
* is returned to the caller so the current executor can switch to it.</p>
*
* <p>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.</p>
+ * {@link #cleanUdfClassLoader(String, long)} on DROP FUNCTION.</p>
*
* @return the {@link UdfClassCache} actually held in the map after this
call —
* either {@code classCache} (we won the race) or the pre-existing
entry
* (another thread won; {@code classCache} has been closed and
must not be used).
*/
- public static UdfClassCache cacheClassLoader(String functionSignature,
UdfClassCache classCache,
- long expirationTime) {
- LOG.info("Cache UDF for: " + functionSignature);
- UdfClassCache existing =
udfLoadedClasses.putIfAbsent(functionSignature, classCache);
+ public static UdfClassCache cacheClassLoader(String functionSignature,
long functionId,
+ UdfClassCache classCache, long expirationTime) {
+ LOG.info("Cache UDF for function signature: {}, function id: {}",
functionSignature, functionId);
+ UdfClassCacheEntry newEntry = new
UdfClassCacheEntry(functionSignature, classCache);
+ UdfClassCacheEntry existing = udfLoadedClasses.putIfAbsent(functionId,
newEntry);
if (existing == null) {
return classCache;
}
// Lost the race against a concurrent first-time load. The cache we
just built has
// never been exposed to any executor, so closing its URLClassLoader
here cannot
// affect anyone. Do NOT touch `existing` — another executor may
already be using it.
try {
- classCache.close();
+ newEntry.classCache.close();
} catch (Exception e) {
- LOG.warn("Failed to close redundant UdfClassCache for " +
functionSignature, e);
+ LOG.warn("Failed to close UdfClassCache for function signature:
{}, function id: {}",
+ newEntry.functionSignature, functionId, e);
}
- return existing;
+ return existing.classCache;
}
- public void cleanUdfClassLoader(String functionSignature) {
- LOG.info("cleanUdfClassLoader for: " + functionSignature);
- UdfClassCache removed = udfLoadedClasses.remove(functionSignature);
+ public void cleanUdfClassLoader(String functionSignature, long functionId)
{
+ LOG.info("cleanUdfClassLoader for function signature: {}, function id:
{}",
+ functionSignature, functionId);
+ UdfClassCacheEntry removed = udfLoadedClasses.remove(functionId);
Review Comment:
[P2] Preserve legacy Java cleanup when the ID is absent
`function_id` is optional, so an older FE can execute a static Java function
on this BE with its positive `TFunction.id` but later send a signature-only
DROP. The callback maps that request to ID 0, and this line removes only
numeric key 0; the real entry has no reclamation path because time eviction is
disabled. This is the current-head inverse of the removed broad-fallback code
discussed in r3860373764: the signature is now not consulted at all, so every
legacy Java cleanup is a no-op. Please retain exact-ID deletion when the field
is present, but provide a collision-safe compatibility or bounded-reclamation
path for omitted IDs and cover it with an unset-ID test.
--
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]
