imbajin commented on code in PR #3329:
URL: https://github.com/apache/hugegraph/pull/3329#discussion_r4275640040
##########
hugegraph-commons/hugegraph-rpc/src/main/java/org/apache/hugegraph/rpc/RpcConsumerConfig.java:
##########
@@ -70,28 +69,119 @@ public <T> T serviceProxy(String interfaceId) {
@Override
public <T> T serviceProxy(String graph, String interfaceId) {
- ConsumerConfig<T> config = this.consumerConfig(graph, interfaceId);
- ConsumerBootstrap<T> bootstrap = Bootstraps.from(config);
- this.bootstraps.add(bootstrap);
- return bootstrap.refer();
+ String serviceId = graph == null ? interfaceId : interfaceId + ":" +
graph;
+ AtomicReference<T> proxy = new AtomicReference<>();
+ this.configs.compute(serviceId, (key, existing) -> {
+ @SuppressWarnings("unchecked")
+ ConsumerConfig<T> config = existing == null ?
this.consumerConfig(graph, interfaceId) :
+ (ConsumerConfig<T>) existing;
+ ConsumerBootstrap<T> bootstrap = Bootstraps.from(config);
+ try {
+ proxy.set(bootstrap.refer());
+ this.bootstraps.add(bootstrap);
+ } catch (RuntimeException | Error failure) {
+ // Retain the cleanup owner even when refer never returned a
proxy.
+ this.bootstraps.add(bootstrap);
+ throwFailure(this.cleanup(bootstrap, failure));
+ }
+ return config;
+ });
+ return proxy.get();
+ }
+
+ @Override
+ public boolean removeServiceProxy(String graph, String interfaceId, Object
proxy) {
+ String serviceId = graph == null ? interfaceId : interfaceId + ":" +
graph;
+ boolean[] removed = {false};
+ this.configs.computeIfPresent(serviceId, (key, config) -> {
+ for (ConsumerBootstrap<?> bootstrap : this.bootstraps) {
+ if (bootstrap.getConsumerConfig() == config &&
bootstrap.getProxyIns() == proxy) {
Review Comment:
[P2] Keep a stable proxy-to-bootstrap identity across failed cleanup retries.
At this head, removeServiceProxy matches the original proxy against
bootstrap.getProxyIns(). In SOFA RPC 5.7.6, DefaultConsumerBootstrap.unRefer()
catches an Exception from cluster.destroy() and then clears proxyIns. If the
explicit cluster.destroy() in cleanup() also fails, this code retains the
bootstrap for retry, but the next removeServiceProxy call with the original
proxy no longer matches it and returns false. StandardHugeGraph's cleanup
Runnable ignores that boolean and is then removed, so graph-scoped cleanup can
report completion while the failed bootstrap is still retained until global
destruction.
Please retain a stable original-proxy/bootstrap association until cleanup
fully succeeds, and add a regression that retries the same removeServiceProxy
after unRefer has cleared proxyIns and cluster destruction initially fails.
Existing failure tests retry owner.destroy(), which does not exercise this
path. This finding is based on current-head source tracing, not an
independently executed native failure test.
Upstream behavior:
https://github.com/sofastack/sofa-rpc/blob/v5.7.6/bootstrap/bootstrap-api/src/main/java/com/alipay/sofa/rpc/bootstrap/DefaultConsumerBootstrap.java#L228-L256
--
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]
