morningman commented on PR #60709:
URL: https://github.com/apache/doris/pull/60709#issuecomment-3980274362
**1. Type coupling in
[ExpiringMap](cci:2://file:///Users/morningman/workspace/git/doris/fe/be-java-extensions/java-common/src/main/java/org/apache/doris/common/jni/utils/ExpiringMap.java:27:0-105:1)**
[ExpiringMap](cci:2://file:///Users/morningman/workspace/git/doris/fe/be-java-extensions/java-common/src/main/java/org/apache/doris/common/jni/utils/ExpiringMap.java:27:0-105:1)
is a generic class `ExpiringMap<K, V>`, but the
[remove()](cci:1://file:///Users/morningman/workspace/git/doris/fe/be-java-extensions/java-common/src/main/java/org/apache/doris/common/jni/utils/ExpiringMap.java:71:4-89:5)
method directly checks `value instanceof UdfClassCache`. This violates the
design principle of generic containers by coupling a general-purpose data
structure to a specific business type. If
[ExpiringMap](cci:2://file:///Users/morningman/workspace/git/doris/fe/be-java-extensions/java-common/src/main/java/org/apache/doris/common/jni/utils/ExpiringMap.java:27:0-105:1)
is used to cache other types of objects in the future, this code will still
perform unnecessary `instanceof` checks.
**Suggestion**: A better approach would be to have
[UdfClassCache](cci:2://file:///Users/morningman/workspace/git/doris/fe/be-java-extensions/java-common/src/main/java/org/apache/doris/common/jni/utils/UdfClassCache.java:28:0-49:1)
implement the `AutoCloseable` interface, and then handle `AutoCloseable`
values uniformly in `ExpiringMap.remove()`:
```java
// UdfClassCache.java
public class UdfClassCache implements AutoCloseable {
// ... existing fields ...
@Override
public void close() {
if (classLoader != null) {
try {
classLoader.close();
} catch (IOException e) {
LOG.warn("Failed to close ClassLoader", e);
}
classLoader = null;
}
}
}
// ExpiringMap.java - remove()
if (value instanceof AutoCloseable) {
try {
((AutoCloseable) value).close();
} catch (Exception e) {
LOG.warn("Failed to close cached resource: " + key, e);
}
}
```
Alternatively, introduce a `RemovalListener<K, V>` callback interface so
that
[ExpiringMap](cci:2://file:///Users/morningman/workspace/git/doris/fe/be-java-extensions/java-common/src/main/java/org/apache/doris/common/jni/utils/ExpiringMap.java:27:0-105:1)
delegates cleanup to the caller.
**2. `classLoader` being `null` when `jarPath` is empty**
In
[getClassCache()](cci:1://file:///Users/morningman/workspace/git/doris/fe/be-java-extensions/java-udf/src/main/java/org/apache/doris/udf/BaseExecutor.java:134:4-169:5),
when `jarPath` is empty (i.e., UDF JAR is in `custom_lib`), the system class
loader is used via `ClassLoader.getSystemClassLoader()`, and the member
variable `classLoader` is never assigned — it remains `null`. Then
`cache.classLoader = classLoader;` also stores `null`. This is functionally
correct since the system class loader should never be closed, but the intent is
not immediately clear. Consider adding a comment to clarify this:
```java
// When jarPath is empty, the UDF is loaded from custom_lib using the system
class loader.
// In this case, classLoader remains null intentionally — the system class
loader
// should not be closed and does not need to be cached.
```
--
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]
