-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
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
@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 的时候么? |
比如有3个SPI实现进行了加载, 在第一个SPI初始化的时候(实现的构造方法中)抛出异常,此时没有catch异常,继续初始化后续的动作,而向上抛出异常,这可能导致因为一个无效插件而导致整个客户端都初始化失败。 一般来说, SPI的构造函数应尽量简单,避免异常(无法保证但是需要考虑),同时留出类似init的方法,用于实际进行复杂的初始化操作(这个基于设置考虑,如果插件实现本身比较简单,不预留也可以),当然init的方法也需要考虑抛出异常的情况。 |
嗯嗯,这个理解。但是目前的实现上面都是调用的无参构造,然后再调用 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.本次修改借助了通义灵码进行了辅助编程