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

Merged
merged 20 commits into from
Oct 15, 2024

Conversation

totalo
Copy link
Contributor

@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
Contributor Author

totalo commented Jun 26, 2024

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

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

ok.

@KomachiSion
Copy link
Collaborator

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

ok.

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

@totalo
Copy link
Contributor Author

totalo commented Jul 3, 2024

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

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

ok.

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

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

@totalo
Copy link
Contributor 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
Contributor 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
Contributor Author

totalo commented Jul 15, 2024

给予了一些review的评价:

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

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

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

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

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

@totalo
Copy link
Contributor 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
Contributor 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
Contributor 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
@totalo
Copy link
Contributor Author

totalo commented Jul 29, 2024

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

@totalo
Copy link
Contributor 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
Contributor 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
Contributor Author

totalo commented Aug 27, 2024

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

@KomachiSion
Copy link
Collaborator

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

比如有3个SPI实现进行了加载, 在第一个SPI初始化的时候(实现的构造方法中)抛出异常,此时没有catch异常,继续初始化后续的动作,而向上抛出异常,这可能导致因为一个无效插件而导致整个客户端都初始化失败。

一般来说, SPI的构造函数应尽量简单,避免异常(无法保证但是需要考虑),同时留出类似init的方法,用于实际进行复杂的初始化操作(这个基于设置考虑,如果插件实现本身比较简单,不预留也可以),当然init的方法也需要考虑抛出异常的情况。

@totalo
Copy link
Contributor Author

totalo commented Oct 10, 2024

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

比如有3个SPI实现进行了加载, 在第一个SPI初始化的时候(实现的构造方法中)抛出异常,此时没有catch异常,继续初始化后续的动作,而向上抛出异常,这可能导致因为一个无效插件而导致整个客户端都初始化失败。

一般来说, SPI的构造函数应尽量简单,避免异常(无法保证但是需要考虑),同时留出类似init的方法,用于实际进行复杂的初始化操作(这个基于设置考虑,如果插件实现本身比较简单,不预留也可以),当然init的方法也需要考虑抛出异常的情况。

嗯嗯,这个理解。但是目前的实现上面都是调用的无参构造,然后再调用 init方法初始化。所有不太明白是为啥。

@KomachiSion
Copy link
Collaborator

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

比如有3个SPI实现进行了加载, 在第一个SPI初始化的时候(实现的构造方法中)抛出异常,此时没有catch异常,继续初始化后续的动作,而向上抛出异常,这可能导致因为一个无效插件而导致整个客户端都初始化失败。
一般来说, SPI的构造函数应尽量简单,避免异常(无法保证但是需要考虑),同时留出类似init的方法,用于实际进行复杂的初始化操作(这个基于设置考虑,如果插件实现本身比较简单,不预留也可以),当然init的方法也需要考虑抛出异常的情况。

嗯嗯,这个理解。但是目前的实现上面都是调用的无参构造,然后再调用 init方法初始化。所有不太明白是为啥。

那就可以了,等合并之后再修改。

@KomachiSion KomachiSion merged commit b971164 into alibaba:develop Oct 15, 2024
2 checks passed
@totalo totalo deleted the patch-12189 branch October 15, 2024 09:23
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.

4 participants