Skip to content
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

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

totalo
Copy link

@totalo totalo commented Jun 23, 2024

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:

  • Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • Format the pull request title like [ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.
  • Run mvn -B clean package apache-rat:check findbugs:findbugs -Dmaven.test.skip=true to make sure basic checks pass. Run mvn clean install -DskipITs to make sure unit-test pass. Run mvn clean test-compile failsafe:integration-test to make sure integration-test pass.

本次修改借助了通义灵码进行了辅助编程

1
2
3
4

@CLAassistant
Copy link

CLAassistant commented Jun 23, 2024

CLA assistant check
All committers have signed the CLA.

@totalo totalo changed the title refactor: Unified Nacos Client address module code [ISSUE #12189] Unified Nacos Client address module code Jun 23, 2024
@KomachiSion
Copy link
Collaborator

暂时还没有进行细节review,目前有一个问题可能需要考虑一下:

#12218 根据这个issue的信息, 目前配置中心和注册中心的ServerListManager 寻址的优先级顺序有一定的区别, 建议在设计时考虑进去。

@totalo
Copy link
Author

totalo commented Jun 26, 2024

暂时还没有进行细节review,目前有一个问题可能需要考虑一下:

#12218 根据这个issue的信息, 目前配置中心和注册中心的ServerListManager 寻址的优先级顺序有一定的区别, 建议在设计时考虑进去。

ok.

@KomachiSion
Copy link
Collaborator

暂时还没有进行细节review,目前有一个问题可能需要考虑一下:
#12218 根据这个issue的信息, 目前配置中心和注册中心的ServerListManager 寻址的优先级顺序有一定的区别, 建议在设计时考虑进去。

ok.

issue中已经有结论回复了, 可以考虑略微进行一下修改,并解决一下出现的冲突。

@totalo
Copy link
Author

totalo commented Jul 3, 2024

暂时还没有进行细节review,目前有一个问题可能需要考虑一下:

#12218 根据这个issue的信息, 目前配置中心和注册中心的ServerListManager 寻址的优先级顺序有一定的区别, 建议在设计时考虑进去。

ok.

issue中已经有结论回复了, 可以考虑略微进行一下修改,并解决一下出现的冲突。

好,感谢大佬(🙏ˊᗜˋ*)

@totalo
Copy link
Author

totalo commented Jul 5, 2024

暂时还没有进行细节review,目前有一个问题可能需要考虑一下:
#12218 根据这个issue的信息, 目前配置中心和注册中心的ServerListManager 寻址的优先级顺序有一定的区别, 建议在设计时考虑进去。

ok.

issue中已经有结论回复了, 可以考虑略微进行一下修改,并解决一下出现的冲突。

已经调整

@KomachiSion
Copy link
Collaborator

暂时还没有进行细节review,目前有一个问题可能需要考虑一下:
#12218 根据这个issue的信息, 目前配置中心和注册中心的ServerListManager 寻址的优先级顺序有一定的区别, 建议在设计时考虑进去。

ok.

issue中已经有结论回复了, 可以考虑略微进行一下修改,并解决一下出现的冲突。

已经调整

好的,近期我会详细review一下,然后提出修改意见。

@totalo
Copy link
Author

totalo commented Jul 9, 2024

暂时还没有进行细节review,目前有一个问题可能需要考虑一下:

#12218 根据这个issue的信息, 目前配置中心和注册中心的ServerListManager 寻址的优先级顺序有一定的区别, 建议在设计时考虑进去。

ok.

issue中已经有结论回复了, 可以考虑略微进行一下修改,并解决一下出现的冲突。

已经调整

好的,近期我会详细review一下,然后提出修改意见。

感谢

Copy link
Collaborator

@KomachiSion KomachiSion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

给予了一些review的评价:

  1. 事件是否也统一抽象出去?
  2. 异步从地址服务器更新地址列表的逻辑是否也可以抽象出去复用?
  3. 梳理一下ServerListManager的构造器,去除一些不使用的,减少复杂度
  4. 是否遗漏了IS_USE_CLOUD_NAMESPACE_PARSING相关逻辑?

@totalo
Copy link
Author

totalo commented Jul 15, 2024

给予了一些review的评价:

  1. 事件是否也统一抽象出去?

  2. 异步从地址服务器更新地址列表的逻辑是否也可以抽象出去复用?

  3. 梳理一下ServerListManager的构造器,去除一些不使用的,减少复杂度

  4. 是否遗漏了IS_USE_CLOUD_NAMESPACE_PARSING相关逻辑?

好的,谢谢大佬,我这边详细看看考虑一下

@totalo
Copy link
Author

totalo commented Jul 17, 2024

@KomachiSion 大佬,我调整了一个版本,辛苦有空 review。主要的问题点如下:

  • load 是通过属性中添加类型与 provider 类型名称匹配,就使用这个配置,这种方式可行吗?

  • IS_USE_CLOUD_NAMESPACE_PARSING 的逻辑原来就没有,这次需要加上吗?

@totalo totalo requested a review from KomachiSion July 17, 2024 16:56
@KomachiSion
Copy link
Collaborator

@KomachiSion 大佬,我调整了一个版本,辛苦有空 review。主要的问题点如下:

  • load 是通过属性中添加类型与 provider 类型名称匹配,就使用这个配置,这种方式可行吗?
  • IS_USE_CLOUD_NAMESPACE_PARSING 的逻辑原来就没有,这次需要加上吗?

关于问题1, 其实这么来思考:

  1. 现有的地址服务器形式的和serverAddr参数形式的算不算是ServerListProvider的一种?如果算的话,为什么他们是在ServerListProvider之外的AddressServerListManager中实现;如果不算的话,那他们和ServerListProvider是什么关系?
  2. 如果同时配置了ServerListProvider的type和endpoint或serverAddr, 他们的优先级是怎样的, 是否互斥?

我个人的拙见:我认为地址服务器也好,配置的serverAddr也好,应该算是provider的一种,和其他用户自定义的Provider一起进行选择;另外,我认为寻址应该是互斥的,而不是互补,因此我赞同使用类似type的方式来指定,不过我觉得可以优化一下方式,比如能解析到有endpoint(无论是properties里有还是环境变量),那就用地址服务器,没有endpoint就用serverAddr,再没有就用用户自定义的Provider(这是个例子,你可以考虑一下是否先用户自定义,再endpoint和serverAddr,但是要保证先endpoint再serverAddr即可)

@totalo
Copy link
Author

totalo commented Jul 19, 2024

@KomachiSion 大佬,我调整了一个版本,辛苦有空 review。主要的问题点如下:

  • load 是通过属性中添加类型与 provider 类型名称匹配,就使用这个配置,这种方式可行吗?
  • IS_USE_CLOUD_NAMESPACE_PARSING 的逻辑原来就没有,这次需要加上吗?

关于问题1, 其实这么来思考:

  1. 现有的地址服务器形式的和serverAddr参数形式的算不算是ServerListProvider的一种?如果算的话,为什么他们是在ServerListProvider之外的AddressServerListManager中实现;如果不算的话,那他们和ServerListProvider是什么关系?
  2. 如果同时配置了ServerListProvider的type和endpoint或serverAddr, 他们的优先级是怎样的, 是否互斥?

我个人的拙见:我认为地址服务器也好,配置的serverAddr也好,应该算是provider的一种,和其他用户自定义的Provider一起进行选择;另外,我认为寻址应该是互斥的,而不是互补,因此我赞同使用类似type的方式来指定,不过我觉得可以优化一下方式,比如能解析到有endpoint(无论是properties里有还是环境变量),那就用地址服务器,没有endpoint就用serverAddr,再没有就用用户自定义的Provider(这是个例子,你可以考虑一下是否先用户自定义,再endpoint和serverAddr,但是要保证先endpoint再serverAddr即可)

@KomachiSion 大佬,我调整了一个版本,辛苦有空 review。主要的问题点如下:

  • load 是通过属性中添加类型与 provider 类型名称匹配,就使用这个配置,这种方式可行吗?
  • IS_USE_CLOUD_NAMESPACE_PARSING 的逻辑原来就没有,这次需要加上吗?

关于问题1, 其实这么来思考:

  1. 现有的地址服务器形式的和serverAddr参数形式的算不算是ServerListProvider的一种?如果算的话,为什么他们是在ServerListProvider之外的AddressServerListManager中实现;如果不算的话,那他们和ServerListProvider是什么关系?
  2. 如果同时配置了ServerListProvider的type和endpoint或serverAddr, 他们的优先级是怎样的, 是否互斥?

我个人的拙见:我认为地址服务器也好,配置的serverAddr也好,应该算是provider的一种,和其他用户自定义的Provider一起进行选择;另外,我认为寻址应该是互斥的,而不是互补,因此我赞同使用类似type的方式来指定,不过我觉得可以优化一下方式,比如能解析到有endpoint(无论是properties里有还是环境变量),那就用地址服务器,没有endpoint就用serverAddr,再没有就用用户自定义的Provider(这是个例子,你可以考虑一下是否先用户自定义,再endpoint和serverAddr,但是要保证先endpoint再serverAddr即可)

嗯嗯,我刚开始逻辑理解的有问题,把endponit 和 severAddr强绑定了。我再调整一下,感谢大佬。

@totalo
Copy link
Author

totalo commented Jul 20, 2024

@KomachiSion 大佬,我又调整了一版。有时间辛苦 review,整体思路如下:

  • 抽出公共的寻址模块,默认分为 AddressServerListProvider 和 EndpointServerListProvider
  • 寻址模块的 SPI 通过order 进行降序排列,这样用户自定义实现的寻址可以通过定义 order 选择合适的位置

@totalo totalo requested a review from KomachiSion July 20, 2024 14:28
Copy link
Collaborator

@KomachiSion KomachiSion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

目前还有几个遗留问题未解决:

  1. 请求地址服务器时的Header还未添加
  2. Provider建议在实际init之前,判断一下当前配置是否选择当前privder进行,比如properties中有endpoint,或parsing=true&环境变量中有ALIBABA_ENDPOINT;如果有的话,就不需要去init serverAddr;或者都init,但是选择的时候不已serverList是否为empty决定。
  3. 地址服务器对象的名称,由于之前不同地址服务器寻址的Manager不同,所以不需要区分名字, 现在复用了相同的父类对象了, 命名上是否需要区分一下。
  4. 配置中心ClientWoker和注册中心获取地址逻辑上有问题,需要修改

待确认问题:

  1. Http template共有的话, 连接池,limiter是否会互相干扰?

目前可以被采纳的部分:

  1. Manager和Provider解耦。
  2. Order被修改成通过接口获取
  3. 保留了旧的大多数Manager接口,使用Manager的地方基本没有修改。

@totalo
Copy link
Author

totalo commented Jul 29, 2024

@KomachiSion 大佬,已经调整,辛苦 review

@totalo
Copy link
Author

totalo commented Aug 1, 2024

@KomachiSion 调整了一下,辛苦大佬再看看~

@totalo totalo requested a review from KomachiSion August 1, 2024 08:27
Copy link
Collaborator

@KomachiSion KomachiSion left a 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问题被阻塞。

@totalo
Copy link
Author

totalo commented Aug 5, 2024

目前初步review似乎没有太大的问题了, 之后会在活动截止前对PR进行整体评判后决定是否合入。

我先跑一下CI,请关注一下结果,避免后续需要合入时由于CI问题被阻塞。

好的,谢谢

@codecov-commenter
Copy link

codecov-commenter commented Aug 23, 2024

Codecov Report

Attention: Patch coverage is 78.83008% with 76 lines in your changes missing coverage. Please review.

Project coverage is 69.64%. Comparing base (62f4a37) to head (013b4f6).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
...cos/client/address/EndpointServerListProvider.java 76.56% 19 Missing and 11 partials ⚠️
...os/client/config/impl/ConfigServerListManager.java 70.58% 16 Missing and 4 partials ⚠️
...acos/client/address/AbstractServerListManager.java 76.31% 6 Missing and 3 partials ⚠️
...os/client/naming/core/NamingServerListManager.java 76.00% 5 Missing and 1 partial ⚠️
...ibaba/nacos/client/address/ServerListProvider.java 0.00% 5 Missing ⚠️
...cos/client/address/AbstractServerListProvider.java 88.88% 1 Missing and 1 partial ⚠️
...acos/client/address/AddressServerListProvider.java 91.30% 1 Missing and 1 partial ⚠️
...alibaba/nacos/client/config/impl/ClientWorker.java 83.33% 1 Missing ⚠️
...a/com/alibaba/nacos/client/constant/Constants.java 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             develop   #12274      +/-   ##
=============================================
- Coverage      69.68%   69.64%   -0.04%     
- Complexity      9421     9431      +10     
=============================================
  Files           1275     1278       +3     
  Lines          41232    41169      -63     
  Branches        4374     4359      -15     
=============================================
- Hits           28731    28674      -57     
+ Misses         10419    10412       -7     
- Partials        2082     2083       +1     
Files with missing lines Coverage Δ
...n/java/com/alibaba/nacos/api/PropertyKeyConst.java 0.00% <ø> (ø)
...ba/nacos/client/address/ServerListChangeEvent.java 100.00% <ø> (ø)
...libaba/nacos/client/config/NacosConfigService.java 95.23% <100.00%> (ø)
...baba/nacos/client/config/http/ServerHttpAgent.java 95.55% <100.00%> (+0.03%) ⬆️
...os/client/config/impl/ConfigHttpClientManager.java 65.62% <100.00%> (-17.71%) ⬇️
...acos/client/config/impl/ConfigTransportClient.java 86.11% <100.00%> (ø)
...acos/client/naming/NacosNamingMaintainService.java 100.00% <100.00%> (ø)
...lient/naming/remote/AbstractNamingClientProxy.java 100.00% <ø> (ø)
...lient/naming/remote/NamingClientProxyDelegate.java 95.45% <100.00%> (ø)
...ient/naming/remote/gprc/NamingGrpcClientProxy.java 98.32% <100.00%> (ø)
... and 12 more

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 981b28f...013b4f6. Read the comment docs.

@totalo
Copy link
Author

totalo commented Aug 27, 2024

@KomachiSion 大佬,其他的做了一个调整,但是这个不是很理解,"未对SPI的实现做异常处理,可能导致其中一个错误的SPI实现抛出异常影响到其他SPI的加载以及整个Client的初始化。", 这是指 init 的时候么?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants