kezhenxu94 commented on code in PR #14797:
URL:
https://github.com/apache/dolphinscheduler/pull/14797#discussion_r1304418083
##########
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:
there is a tricky issue here, if there are 2 (or more) calls to this method
`getProxyClient` with the same `serverHost`, the `proxyClientCache` would
return two different `ConcurrentHashMap`, then weird bug might occur.
Please check the javadoc of the `com.google.common.cache.Cache#get`,
> Warning: For any given key, every loader used with it should compute the
same value. Otherwise, a call that passes one loader may return the result of
another call with a differently behaving loader. For example, a call that
requests a short timeout for an RPC may wait for a similar call that requests a
long timeout, or a call by an unprivileged user may return a resource
accessible only to a privileged user making a similar call. To prevent this
problem, create a key object that includes all values that affect the result of
the query. Or use LoadingCache.get(K), which lacks the ability to refer to
state other than that in the key.
--
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]
