-
Notifications
You must be signed in to change notification settings - Fork 26.6k
feat(registry): support the option, whether to check the connection … #15594
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 3.3 #15594 +/- ##
============================================
Coverage 61.06% 61.07%
+ Complexity 11700 15 -11685
============================================
Files 1924 1924
Lines 87086 87096 +10
Branches 13115 13118 +3
============================================
+ Hits 53182 53192 +10
Misses 28460 28460
Partials 5444 5444
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
...bbo-registry-nacos/src/main/java/org/apache/dubbo/registry/nacos/NacosConnectionManager.java
Outdated
Show resolved
Hide resolved
...y-zookeeper/src/main/java/org/apache/dubbo/registry/zookeeper/ZookeeperServiceDiscovery.java
Outdated
Show resolved
Hide resolved
| throw e; | ||
| } | ||
| nacosConnectionManager = | ||
| new NacosConnectionManager(connectionURL, false, retryTimes, sleepMsBetweenRetries); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change might cause some problems. This registry isn’t functioning normally, but it's still added to the RegistryManager. As a result, subsequent actions might use this registry, which could lead to more errors. I'm not sure exactly what will happen after this PR is merged, but I’ll try to reproduce the issue later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After trying it out, I got the result.
23:41:19.981 |-WARN [DubboRegistryRetryTimer-thread-1] registry.nacos.NacosNamingServiceWrapper: -| [DUBBO] Failed to request nacos naming server. Dubbo will try to retry in 10. Try times: 6, dubbo version: 3.3.6-SNAPSHOT, current host: 192.168.31.224, error code: 1-37. This may be caused by , go to https://dubbo.apache.org/faq/1/37 to find instructions.
com.alibaba.nacos.api.exception.NacosException: Client not connected, current status:STARTING
at com.alibaba.nacos.common.remote.client.RpcClient.request(RpcClient.java:645) ~[nacos-client-2.5.1.jar:?]
at com.alibaba.nacos.common.remote.client.RpcClient.request(RpcClient.java:624) ~[nacos-client-2.5.1.jar:?]
at com.alibaba.nacos.client.naming.remote.gprc.NamingGrpcClientProxy.requestToServer(NamingGrpcClientProxy.java:449) ~[nacos-client-2.5.1.jar:?]
at com.alibaba.nacos.client.naming.remote.gprc.NamingGrpcClientProxy.doRegisterService(NamingGrpcClientProxy.java:252) ~[nacos-client-2.5.1.jar:?]
at com.alibaba.nacos.client.naming.remote.gprc.NamingGrpcClientProxy.registerServiceForEphemeral(NamingGrpcClientProxy.java:147) ~[nacos-client-2.5.1.jar:?]
at com.alibaba.nacos.client.naming.remote.gprc.NamingGrpcClientProxy.registerService(NamingGrpcClientProxy.java:138) ~[nacos-client-2.5.1.jar:?]
at com.alibaba.nacos.client.naming.remote.NamingClientProxyDelegate.registerService(NamingClientProxyDelegate.java:96) ~[nacos-client-2.5.1.jar:?]
at com.alibaba.nacos.client.naming.NacosNamingService.registerInstance(NacosNamingService.java:161) ~[nacos-client-2.5.1.jar:?]
at org.apache.dubbo.registry.nacos.NacosNamingServiceWrapper.lambda$registerInstance$5(NacosNamingServiceWrapper.java:133) ~[dubbo-3.3.6-SNAPSHOT.jar:3.3.6-SNAPSHOT]
at org.apache.dubbo.registry.nacos.NacosNamingServiceWrapper.accept(NacosNamingServiceWrapper.java:495) ~[dubbo-3.3.6-SNAPSHOT.jar:3.3.6-SNAPSHOT]
at org.apache.dubbo.registry.nacos.NacosNamingServiceWrapper.registerInstance(NacosNamingServiceWrapper.java:133) ~[dubbo-3.3.6-SNAPSHOT.jar:3.3.6-SNAPSHOT]
at org.apache.dubbo.registry.nacos.NacosRegistry.doRegister(NacosRegistry.java:205) ~[dubbo-3.3.6-SNAPSHOT.jar:3.3.6-SNAPSHOT]
at org.apache.dubbo.registry.retry.FailedRegisteredTask.doRetry(FailedRegisteredTask.java:33) ~[dubbo-3.3.6-SNAPSHOT.jar:3.3.6-SNAPSHOT]
at org.apache.dubbo.registry.retry.AbstractRetryTask.run(AbstractRetryTask.java:127) ~[dubbo-3.3.6-SNAPSHOT.jar:3.3.6-SNAPSHOT]
at org.apache.dubbo.common.timer.HashedWheelTimer$HashedWheelTimeout.expire(HashedWheelTimer.java:654) ~[dubbo-3.3.6-SNAPSHOT.jar:3.3.6-SNAPSHOT]
at org.apache.dubbo.common.timer.HashedWheelTimer$HashedWheelBucket.expireTimeouts(HashedWheelTimer.java:733) ~[dubbo-3.3.6-SNAPSHOT.jar:3.3.6-SNAPSHOT]
at org.apache.dubbo.common.timer.HashedWheelTimer$Worker.run(HashedWheelTimer.java:455) ~[dubbo-3.3.6-SNAPSHOT.jar:3.3.6-SNAPSHOT]
at java.base/java.lang.Thread.run(Thread.java:1583) [?:?]
It will return to normal once the Nacos server is back online.
It's different from metadata or the config center — I think they should follow the same behavior.
This needs some discussion.
RainYuY
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! Please take a look at these comments.
|
Thank you very mush for your preview. In the zookeeper registry client, i have modified it to use Can it be considered that And then i will evaluate the impact of this change on the config center and metadata center. |
The modification is OK, and I even think the nacos.check configuration item could be removed in the future. Also, what I mentioned earlier was not that this PR affects the config center or metadata, but that their behavior is inconsistent. In the current implementation of this PR, even if the check fails, the registry is still initialized. As a result, the service keeps trying to register to an unavailable registry. I think a better approach would be not to repeatedly attempt registration when the registry is unavailable. Instead, we should first check the availability of the registry, and only proceed with registration once it becomes available. |
@RainYuY Your focus is right. I have checked the startup process of the config-center and metadata. they both support the
About this situation, is it possible to add connection status check logic before register?And the retry task should be retained. |
I think there will be a lot of work to do. The best practice might be to open a new PR to reproduce it.
Yes, I think this is the correct action.And it should be solved at this PR. |
@RainYuY It's a wise decision to create a new issue. In addition to making the ConfigCenter and MetadataCenter support |
|
LGTM |
|
duplicated with #15639 |
|
@fantiq cc conflicts |
RainYuY
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
...st/java/org/apache/dubbo/remoting/zookeeper/curator5/Curator5ZookeeperClientManagerTest.java
Show resolved
Hide resolved
zrlw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What is the purpose of the change?
resolve #15271 #15003 , this issues are related to the config option
registry.check.When
registry.check=true(default value istrue) is set, the connection status will be checked when creating the registry client. if the check fails, an exception will be thrown.when
registry.check=falseis set, no check is performed and no exception is ever thrown.The reason for NEP is caused by setting the option
registry.checktofalseand throwning an exception.so here when
registry.check=falseno exception should be thrown. that is, the connection status should not be checked.Currently, in the nacos client, the option
nacos.checkis used to determine whether to check the connection status or not.the zookeeper client does not yet support not checking the connection status.
This PR add an new option
zookeeper.check, it is similar to optionnacos.check.When creating a registry client, it will first be created using the default behavior. if an exception is thrown and
registry.check=false,it will try to create the registry client again by turning on the option (
nacos.check=falseorzookeeper.check=false) to not check the connection status.Checklist