ruanwenjun commented on code in PR #14797:
URL:
https://github.com/apache/dolphinscheduler/pull/14797#discussion_r1304475784
##########
dolphinscheduler-extract/dolphinscheduler-extract-base/src/main/java/org/apache/dolphinscheduler/extract/base/client/JdkDynamicRpcClientProxyFactory.java:
##########
@@ -21,31 +21,44 @@
import org.apache.dolphinscheduler.extract.base.utils.Host;
import java.lang.reflect.Proxy;
+import java.time.Duration;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
+import lombok.SneakyThrows;
+
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+
/**
* This class is used to create a proxy client which will transform local
method invocation to remove invocation.
*/
public class JdkDynamicRpcClientProxyFactory implements IRpcClientProxyFactory
{
private final NettyRemotingClient nettyRemotingClient;
- // todo: use guava cache to avoid memory leak
- private final Map<String, Map<String, Object>> proxyClientCache = new
ConcurrentHashMap<>();
+ private final Cache<String, Map<String, Object>> proxyClientCache =
CacheBuilder.newBuilder()
+ // expire here to remove dead host
+ .expireAfterAccess(Duration.ofHours(1))
+ .build();
public JdkDynamicRpcClientProxyFactory(NettyRemotingClient
nettyRemotingClient) {
this.nettyRemotingClient = nettyRemotingClient;
}
+ @SneakyThrows
@SuppressWarnings("unchecked")
@Override
public <T> T getProxyClient(String serverHost, Class<T> clientInterface) {
- return (T) proxyClientCache
- .computeIfAbsent(serverHost, key -> new ConcurrentHashMap<>())
- .computeIfAbsent(clientInterface.getName(),
- key -> Proxy.newProxyInstance(
- clientInterface.getClassLoader(), new
Class[]{clientInterface},
- new
ClientInvocationHandler(Host.of(serverHost), nettyRemotingClient)));
+ return (T) proxyClientCache.get(serverHost, ConcurrentHashMap::new)
Review Comment:
Thanks for reminding me of this, I tested this, and this should work well.
Go deep into the code,
```java
V get(K key, Callable<? extends V> loader) throws ExecutionException;
```
I find the doc might means if you have the same keys with different loader,
e.g
```
cache.get("key", loader1);
cache.get("key", loader2);
```
and the two methods occur in two threads, the return value is unknown, since
the return value is related to the loader, the first method might return the
value of loader2, but if we use LoadingCache, the return value is known, since
there is only one loader, the loader is not expose to the get method.
Anyway I changed to LoadingCache, it's safer.
--
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]
