-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
[ISSUE #12189] Unified Nacos Client address module code #12274
base: develop
Are you sure you want to change the base?
Conversation
…m expansion capabilities
暂时还没有进行细节review,目前有一个问题可能需要考虑一下: #12218 根据这个issue的信息, 目前配置中心和注册中心的ServerListManager 寻址的优先级顺序有一定的区别, 建议在设计时考虑进去。 |
ok. |
issue中已经有结论回复了, 可以考虑略微进行一下修改,并解决一下出现的冲突。 |
好,感谢大佬(🙏ˊᗜˋ*) |
已经调整 |
好的,近期我会详细review一下,然后提出修改意见。 |
感谢 |
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.
给予了一些review的评价:
- 事件是否也统一抽象出去?
- 异步从地址服务器更新地址列表的逻辑是否也可以抽象出去复用?
- 梳理一下ServerListManager的构造器,去除一些不使用的,减少复杂度
- 是否遗漏了IS_USE_CLOUD_NAMESPACE_PARSING相关逻辑?
client/src/main/java/com/alibaba/nacos/client/address/AbstractServerListManager.java
Outdated
Show resolved
Hide resolved
client/src/main/java/com/alibaba/nacos/client/address/AbstractServerListManager.java
Show resolved
Hide resolved
client/src/main/java/com/alibaba/nacos/client/address/AbstractServerListManager.java
Outdated
Show resolved
Hide resolved
client/src/main/java/com/alibaba/nacos/client/config/impl/ConfigServerListChangeEvent.java
Outdated
Show resolved
Hide resolved
client/src/main/java/com/alibaba/nacos/client/naming/event/NamingServerListChangeEvent.java
Outdated
Show resolved
Hide resolved
client/src/main/java/com/alibaba/nacos/client/naming/core/NamingServerListManager.java
Outdated
Show resolved
Hide resolved
好的,谢谢大佬,我这边详细看看考虑一下 |
@KomachiSion 大佬,我调整了一个版本,辛苦有空 review。主要的问题点如下:
|
client/src/main/java/com/alibaba/nacos/client/address/AbstractServerListManager.java
Outdated
Show resolved
Hide resolved
client/src/main/java/com/alibaba/nacos/client/address/AbstractServerListManager.java
Outdated
Show resolved
Hide resolved
client/src/main/java/com/alibaba/nacos/client/address/ServerListHttpClientManager.java
Outdated
Show resolved
Hide resolved
关于问题1, 其实这么来思考:
我个人的拙见:我认为地址服务器也好,配置的serverAddr也好,应该算是provider的一种,和其他用户自定义的Provider一起进行选择;另外,我认为寻址应该是互斥的,而不是互补,因此我赞同使用类似type的方式来指定,不过我觉得可以优化一下方式,比如能解析到有endpoint(无论是properties里有还是环境变量),那就用地址服务器,没有endpoint就用serverAddr,再没有就用用户自定义的Provider(这是个例子,你可以考虑一下是否先用户自定义,再endpoint和serverAddr,但是要保证先endpoint再serverAddr即可) |
嗯嗯,我刚开始逻辑理解的有问题,把endponit 和 severAddr强绑定了。我再调整一下,感谢大佬。 |
@KomachiSion 大佬,我又调整了一版。有时间辛苦 review,整体思路如下:
|
client/src/main/java/com/alibaba/nacos/client/address/AbstractServerListManager.java
Outdated
Show resolved
Hide resolved
client/src/main/java/com/alibaba/nacos/client/address/AddressServerListProvider.java
Outdated
Show resolved
Hide resolved
client/src/main/java/com/alibaba/nacos/client/address/EndpointServerListProvider.java
Outdated
Show resolved
Hide resolved
client/src/main/java/com/alibaba/nacos/client/address/ServerListHttpClientManager.java
Outdated
Show resolved
Hide resolved
client/src/main/java/com/alibaba/nacos/client/naming/remote/http/NamingHttpClientProxy.java
Outdated
Show resolved
Hide resolved
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.
目前还有几个遗留问题未解决:
- 请求地址服务器时的Header还未添加
- Provider建议在实际init之前,判断一下当前配置是否选择当前privder进行,比如properties中有endpoint,或parsing=true&环境变量中有ALIBABA_ENDPOINT;如果有的话,就不需要去init serverAddr;或者都init,但是选择的时候不已serverList是否为empty决定。
- 地址服务器对象的名称,由于之前不同地址服务器寻址的Manager不同,所以不需要区分名字, 现在复用了相同的父类对象了, 命名上是否需要区分一下。
- 配置中心ClientWoker和注册中心获取地址逻辑上有问题,需要修改
待确认问题:
- Http template共有的话, 连接池,limiter是否会互相干扰?
目前可以被采纳的部分:
- Manager和Provider解耦。
- Order被修改成通过接口获取
- 保留了旧的大多数Manager接口,使用Manager的地方基本没有修改。
client/src/main/java/com/alibaba/nacos/client/address/AbstractServerListManager.java
Outdated
Show resolved
Hide resolved
client/src/main/java/com/alibaba/nacos/client/address/EndpointServerListProvider.java
Outdated
Show resolved
Hide resolved
client/src/main/java/com/alibaba/nacos/client/address/ServerListHttpClientManager.java
Outdated
Show resolved
Hide resolved
client/src/main/java/com/alibaba/nacos/client/config/impl/ClientWorker.java
Outdated
Show resolved
Hide resolved
client/src/main/java/com/alibaba/nacos/client/config/impl/ClientWorker.java
Outdated
Show resolved
Hide resolved
client/src/main/java/com/alibaba/nacos/client/naming/remote/http/NamingHttpClientProxy.java
Outdated
Show resolved
Hide resolved
@KomachiSion 大佬,已经调整,辛苦 review |
client/src/main/java/com/alibaba/nacos/client/address/AbstractServerListManager.java
Show resolved
Hide resolved
client/src/main/java/com/alibaba/nacos/client/config/impl/ConfigHttpClientManager.java
Outdated
Show resolved
Hide resolved
@KomachiSion 调整了一下,辛苦大佬再看看~ |
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.
目前初步review似乎没有太大的问题了, 之后会在活动截止前对PR进行整体评判后决定是否合入。
我先跑一下CI,请关注一下结果,避免后续需要合入时由于CI问题被阻塞。
好的,谢谢 |
@KomachiSion 大佬,其他的做了一个调整,但是这个不是很理解,"未对SPI的实现做异常处理,可能导致其中一个错误的SPI实现抛出异常影响到其他SPI的加载以及整个Client的初始化。", 这是指 init 的时候么? |
for #12189
What is the purpose of the change
Unified Nacos Client address module code, and provide custom expansion capabilities.
Brief changelog
XX
Verifying this change
XXXX
Follow this checklist to help us incorporate your contribution quickly and easily:
[ISSUE #123] Fix UnknownException when host config not exist
. Each commit in the pull request should have a meaningful subject line and body.mvn -B clean package apache-rat:check findbugs:findbugs -Dmaven.test.skip=true
to make sure basic checks pass. Runmvn clean install -DskipITs
to make sure unit-test pass. Runmvn clean test-compile failsafe:integration-test
to make sure integration-test pass.本次修改借助了通义灵码进行了辅助编程