[ISSUE #12189] refactor the server list manager code in the client module #12366

misakacoder · 2024-07-17T09:25:32Z

What is the purpose of the change

#12189

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.

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

…ient module

…-all into refactor-12189

KomachiSion · 2024-07-18T07:55:05Z

已收到PR，会尽快进行review，请时刻关注

KomachiSion

方案中可以认可的点：

用SPI进行扩展。
将AddressServer和Properties独立作为两个拓展实现。
将公共逻辑抽象到抽象类中

可能有问题的点：

SPI不需要自己实现，我理解jdk自带的足以支持
order的排序方式可以写入到AbstractServerListManager，作为方法提供，用注解+反射的方法可能没那么好。
事件不需要修改名称，旧的名称足以表达意思
地址服务器的实现可能有逻辑遗漏，建议在梳理一下，做好注册和配置中心的区分（命名和调用逻辑上。

KomachiSion · 2024-07-19T08:17:20Z

api/src/main/java/com/alibaba/nacos/api/PropertyKeyConst.java

@@ -52,6 +52,8 @@ public class PropertyKeyConst {
 public static final String RAM_ROLE_NAME = "ramRoleName";

 public static final String SERVER_ADDR = "serverAddr";
+
+ public static final String SERVER_FILE = "serverFile";


这个Server file是什么？

这个Server file是什么？

这个是我参考的MemberLookup的实现逻辑，多加了一个，如果觉得暂时没有必要可以去掉

先去掉吧，本issue和pr以重构和留出拓展接口为主，如果想把这个实现贡献到社区，可以考虑这个PR合并之后，再添加一个issue和PR来支持。

KomachiSion · 2024-07-19T08:29:24Z

client/src/main/java/com/alibaba/nacos/client/address/factory/ServerListManagerFactory.java

+ try {
+ String managerName = cls.getSimpleName();
+ Constructor<AbstractServerListManager> constructor = cls.getConstructor(NacosClientProperties.class);
+ AbstractServerListManager serverListManager = constructor.newInstance(clientProperties);


如果用SPI的话，最好使用无参数构造器，然后调用init方法的方式处理，否则需要构建出两次实例。

如果用SPI的话，最好使用无参数构造器，然后调用init方法的方式处理，否则需要构建出两次实例。

就是jdk的SPI不支持有参构造，和会自动实例化类，我才手动实现了一个，你看看嘛，你觉得没有必要我就先去掉

KomachiSion · 2024-07-19T08:30:27Z

client/src/main/java/com/alibaba/nacos/client/address/factory/ServerListManagerFactory.java

+
+ private static int getOrder(Class<AbstractServerListManager> cls) {
+ Order order = cls.getAnnotation(Order.class);
+ return order != null ? order.value() : Integer.MAX_VALUE;


同样不建议用反射在获取。可以使用getOrder方法替代。

KomachiSion · 2024-07-19T08:36:06Z

client/src/main/java/com/alibaba/nacos/client/address/factory/ServerListManagerFactory.java

+ comparator = comparator != null ? comparator : defaultComparator;
+ List<Class<AbstractServerListManager>> classes = ServiceLoader.load(AbstractServerListManager.class);
+ classes.sort(comparator);
+ for (Class<AbstractServerListManager> cls : classes) {


这种循环的话，好像和旧的逻辑会有区别。

之前的逻辑，如果endpoint不为空，但是从endpoint的地址服务器上获取到空列表的话，会直接报错serverlist是empty。

但是这个逻辑似乎会出现去使用serverAddr的值的情况。

这种循环的话，好像和旧的逻辑会有区别。

之前的逻辑，如果endpoint不为空，但是从endpoint的地址服务器上获取到空列表的话，会直接报错serverlist是empty。

但是这个逻辑似乎会出现去使用serverAddr的值的情况。

我之前考虑的是按顺序搜寻可用的nacos地址，如果最终所有寻址方式都不能搜寻到可用的nacos服务，就抛异常。
如果和之前保持一直的话，我这样实现，如果每种寻址方式的特定配置，比如endpoint，serverAddr配置了，但是没有获取到nacos服务列表，就直接抛出异常，这样怎么样呢

KomachiSion · 2024-07-19T08:41:19Z

client/src/main/java/com/alibaba/nacos/client/address/impl/ServerListUpdatedEvent.java

 */
-public class ServerListChangeEvent extends SlowEvent {


不用修改事件名吧

KomachiSion · 2024-07-19T08:42:49Z

client/src/main/java/com/alibaba/nacos/client/address/spi/ServiceLoader.java

+ *
+ * @author misakacoder
+ */
+public class ServiceLoader {


不需要自己实现ServiceLoader， java本身有实现。如果jdk本身的不符合需求，也有其他社区优秀的实现，避免重复造轮子。

KomachiSion · 2024-07-19T08:48:44Z

client/src/main/java/com/alibaba/nacos/client/address/impl/AddressServerListManager.java

+ }
+
+ @Override
+ protected String initServerName(NacosClientProperties properties) {


如何区分是注册中心模块还是配置中心模块呢？

如何区分是注册中心模块还是配置中心模块呢？

原来的代码，只有配置中心的ServerListManager才有initServerName方法，最终暴露出去的是getName方法，作用是生成FailoverFile和SnapshotFile，注册中心没有这个哦

但是重构之后，注册和配置中心都会复用这个name的代码，可能会生成出一样的名字。

KomachiSion · 2024-07-19T08:50:41Z

client/src/main/java/com/alibaba/nacos/client/address/impl/AddressServerListManager.java

+
+ private List<String> readServerList() {
+ try {
+ HttpRestResult<String> result = nacosRestTemplate.get(addressServerUrl, Header.EMPTY, Query.EMPTY, String.class);


这里会丢失注册中心和配置中心的区别。注册中心有一个header

Header header = NamingHttpUtil.builderHeader();

这里会丢失注册中心和配置中心的区别。注册中心有一个header

Header header = NamingHttpUtil.builderHeader();

这个具体看下我下面的回复，我理解的是注册中心和配置中心是一样的，就是请求url，获取响应得到nacos服务列表

这个Header很重要，里面会带一个Request-Module的信息，用于区分需要注册中心的地址还是配置中心的。

KomachiSion · 2024-07-19T08:56:37Z

client/src/main/java/com/alibaba/nacos/client/address/impl/FileServerListManager.java

+ * @author misakacoder
+ */
+@Order(200)
+public class FileServerListManager extends AbstractServerListManager {


添加新增的寻址扩展方法，可以在后续增加， PR还是聚焦在重构上。

KomachiSion · 2024-07-19T08:58:24Z

client/src/main/java/com/alibaba/nacos/client/config/http/ServerHttpAgent.java


 /**
 * Server Agent.
 *
 * @author water.lyl
 */
 public class ServerHttpAgent implements HttpAgent {
- 
+


请使用nacos code style进行reformat，不需要修改缩进。

misakacoder · 2024-07-19T09:24:05Z

方案中可以认可的点：

用SPI进行扩展。

将AddressServer和Properties独立作为两个拓展实现。

将公共逻辑抽象到抽象类中

可能有问题的点：

SPI不需要自己实现，我理解jdk自带的足以支持

order的排序方式可以写入到AbstractServerListManager，作为方法提供，用注解+反射的方法可能没那么好。

事件不需要修改名称，旧的名称足以表达意思

地址服务器的实现可能有逻辑遗漏，建议在梳理一下，做好注册和配置中心的区分（命名和调用逻辑上。

jdk自带的SPI会自动创建全部实现类的实例，因为我的初始化逻辑是写到构造方法里面的，所以我不想一次性全部实例化。当然像上面说的，采用init函数来手动初始化也行，我改下吧
自定义排序我弄成方法，的确能不反射就不反射。
这个我改回去，我是把之前的2个不同事件名称删除了重新建的，命名习惯不一样。
我看过config和naming的ServerListManager逻辑，我理解的地址服务器就是简单的一个web服务，请求指定接口返回nacos服务列表，就如官网的图所示，所以像上面说的Header header = NamingHttpUtil.builderHeader();这个区别，我认为可能是不同的人写的导致的，因为就是请求一个接口，返回nacos服务器地址列表，config和naming不应该有区别才对噻。

misakacoder · 2024-07-19T09:54:23Z

@KomachiSion 幸苦空了看看我的回复和疑惑，我这边好调整调整

KomachiSion · 2024-07-23T07:18:39Z

方案中可以认可的点：

用SPI进行扩展。

将AddressServer和Properties独立作为两个拓展实现。

将公共逻辑抽象到抽象类中

可能有问题的点：

SPI不需要自己实现，我理解jdk自带的足以支持

order的排序方式可以写入到AbstractServerListManager，作为方法提供，用注解+反射的方法可能没那么好。

事件不需要修改名称，旧的名称足以表达意思

地址服务器的实现可能有逻辑遗漏，建议在梳理一下，做好注册和配置中心的区分（命名和调用逻辑上。

jdk自带的SPI会自动创建全部实现类的实例，因为我的初始化逻辑是写到构造方法里面的，所以我不想一次性全部实例化。当然像上面说的，采用init函数来手动初始化也行，我改下吧

自定义排序我弄成方法，的确能不反射就不反射。

这个我改回去，我是把之前的2个不同事件名称删除了重新建的，命名习惯不一样。

我看过config和naming的ServerListManager逻辑，我理解的地址服务器就是简单的一个web服务，请求指定接口返回nacos服务列表，就如官网的图所示，所以像上面说的Header header = NamingHttpUtil.builderHeader();这个区别，我认为可能是不同的人写的导致的，因为就是请求一个接口，返回nacos服务器地址列表，config和naming不应该有区别才对噻。

第4点很重要，是用来区分注册中心和配置中心的情况，如果是用同一个地址服务器，然后注册和配置中心是不同集群的时候，需要通过Header来区分是需要哪个。

KomachiSion · 2024-07-23T07:19:41Z

api/src/main/java/com/alibaba/nacos/api/PropertyKeyConst.java

@@ -52,6 +52,8 @@ public class PropertyKeyConst {
 public static final String RAM_ROLE_NAME = "ramRoleName";

 public static final String SERVER_ADDR = "serverAddr";
+
+ public static final String SERVER_FILE = "serverFile";


先去掉吧，本issue和pr以重构和留出拓展接口为主，如果想把这个实现贡献到社区，可以考虑这个PR合并之后，再添加一个issue和PR来支持。

KomachiSion · 2024-07-23T07:21:14Z

client/src/main/java/com/alibaba/nacos/client/address/impl/AddressServerListManager.java

+ }
+
+ @Override
+ protected String initServerName(NacosClientProperties properties) {


但是重构之后，注册和配置中心都会复用这个name的代码，可能会生成出一样的名字。

KomachiSion · 2024-07-23T07:21:48Z

client/src/main/java/com/alibaba/nacos/client/address/impl/AddressServerListManager.java

+
+ private List<String> readServerList() {
+ try {
+ HttpRestResult<String> result = nacosRestTemplate.get(addressServerUrl, Header.EMPTY, Query.EMPTY, String.class);


这个Header很重要，里面会带一个Request-Module的信息，用于区分需要注册中心的地址还是配置中心的。

KomachiSion · 2024-07-23T07:22:28Z

@KomachiSion 幸苦空了看看我的回复和疑惑，我这边好调整调整

辛苦再调整一下。

KomachiSion · 2024-07-29T03:30:04Z

@misakacoder any update？

…ient module

misakacoder · 2024-07-29T09:56:27Z

@misakacoder any update？

不好意思，前几天有点忙，我调整了下，幸苦帮忙review下呢

KomachiSion

辛苦再看下评论，进行一下修改。

大体上没有问题了。一些细节问题处理一下。

KomachiSion · 2024-08-01T05:49:18Z

api/src/main/java/com/alibaba/nacos/api/PropertyKeyConst.java

@@ -52,7 +52,7 @@ public class PropertyKeyConst {
 public static final String RAM_ROLE_NAME = "ramRoleName";

 public static final String SERVER_ADDR = "serverAddr";
- 
+


请使用nacos codeStyle进行格式化，无需修改缩进

KomachiSion · 2024-08-01T05:55:53Z

client/src/main/java/com/alibaba/nacos/client/address/manager/AbstractServerListManager.java

+ String providerName = provider.getClass().getSimpleName();
+ provider.startup(properties, namespace, getModuleType());
+ List<String> serverList = provider.getServerList();
+ if (CollectionUtils.isNotEmpty(serverList)) {


这个地方我好像评论过，如果只以是否为空的情况判断，可能会造成不符合预期的情况：

endpoint和serverAddr都传递，但是从endpoint处获取到地址列表为空时，旧版本是抛出错误，认为当前没有可用地址，但是心逻辑是用了serverAddr。

看下是有有个接口，比如isApplied或者类似的，表达这个provider在当前配置下是否应该使用，以此来决定是否使用这个provider，而不是通过serverlist来判断。

其他部分逻辑应该比较完善了

KomachiSion · 2024-08-01T06:00:02Z

client/src/main/java/com/alibaba/nacos/client/address/manager/AbstractServerListManager.java

+ }
+
+ private void updateServerList(List<String> serverList) {
+ if (serverList == null || serverList.isEmpty() || serverList.equals(this.serverList)) {


这里我确认一下， this.serverList经过repairServerAddr之后，和原本的list可能会不一致，会不会导致不停的更新的情况。

另外这里注意一下顺序问题，比如1.1.1.1,2.2.2.2.和 2.2.2.2,1.1.1.1 是否需要更新。

KomachiSion · 2024-08-01T06:08:57Z

client/src/main/java/com/alibaba/nacos/client/config/impl/ClientWorker.java

- boolean isFixServer = agent.serverListManager.isFixed;
+ AbstractServerListManager serverListManager = agent.serverListManager;
+ ServerListProvider serverListProvider = serverListManager.getServerListProvider();
+ boolean isFixServer = serverListProvider instanceof PropertiesServerListProvider;


这里也是建议将这个属性抽象成serverListProvider的方法，比如isFixedServer 或者 isDynamicServer 等能表达类似意思的。

否则这里使用处还要感知具体类型，不是很好。

当然可以从AbstractServerListManager暴露也行，AbstractServerListManager不是有判断是否需要构造线程池来更新地址吗？

@KomachiSion 大佬，我优化了一下，麻烦再review一下呢

KomachiSion · 2024-08-01T06:10:15Z

client/src/main/java/com/alibaba/nacos/client/config/impl/ClientWorker.java

- metric.put("serverUrls", agent.serverListManager.getUrlString());
+ if (serverListProvider instanceof AddressServerListProvider) {
+ metric.put("addressUrl", ReflectUtils.getFieldValue(serverListProvider, "addressServerUrl"));
+ }


同上，可以配合上面那个接口，一起处理。

KomachiSion · 2024-08-01T06:13:36Z

client/src/main/java/com/alibaba/nacos/client/naming/remote/http/NamingHttpClientProxy.java

- NAMING_LOGGER.debug("request {} failed.", nacosDomain, e);
- }
- }
- }


这段逻辑的移除，会不会导致仅有一个地址的时候不进行重试了？

…-all into refactor-12189

misakacoder · 2024-08-02T10:01:16Z

@KomachiSion 大佬，我优化了一下，麻烦再review一下呢

KomachiSion

两个建议：

关于ServerHttpAgent文件的改动，我觉得最好不要进行大改，保持原有逻辑，因为我们是寻址模块的重构，而不是Agent的重构。如果有兴趣，可以在这个PR结束之后，单独提交。
Provider的startup其实我建议在valid之后，因为地址服务器以及可能后续的用户自定义Provider，都可能在startup中有网络或IO操作，这可能导致耗时以及报错异常等，如果放在valid之前，花费时间以及内存进行了多项初始化工作，甚至是IO操作，然后不满足valid条件被忽略，有点得不偿失。

另外关于NamingHttpClientProxy丢失的逻辑，依然没有补回。

KomachiSion · 2024-08-05T02:27:57Z

client/src/main/java/com/alibaba/nacos/client/address/manager/ConfigServerListManager.java

+ public String getAddressServerUrl() {
+ String addressServerUrl = null;
+ if (serverListProvider instanceof AddressServerListProvider) {
+ addressServerUrl = ReflectUtils.getFieldValue(serverListProvider, "addressServerUrl").toString();


用反射是不是不太好？

是不是改为provider提供一个getRemoteProviderUrl方法来获取比较好，默认这个方法可以返回一个空字符串，或者返回一个Optional.

misakacoder · 2024-08-05T09:34:16Z

@KomachiSion 已修改，麻烦再review下呢，另外NamingHttpClientProxy只有一个地址就不重试的逻辑我是优化成这样了。

KomachiSion

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

misakacoder · 2024-08-07T07:19:17Z

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

好的，感谢

misakacoder added 2 commits July 17, 2024 17:12

[ISSUE alibaba#12189] refactor the server list manager code in the cl…

e4020fc

…ient module

add class author

22637c0

KomachiSion added the Summer label Jul 18, 2024

misakacoder and others added 3 commits July 18, 2024 12:08

fix ServerHttpAgent cannot find context path

34ddbd9

Merge branch 'alibaba:develop' into refactor-12189

fd180f3

Merge branch 'refactor-12189' of https://github.com/misakacoder/nacos…

21ff9ea

…-all into refactor-12189

KomachiSion requested changes Jul 19, 2024

View reviewed changes

KomachiSion requested changes Jul 23, 2024

View reviewed changes

misakacoder and others added 2 commits July 29, 2024 12:06

Merge branch 'alibaba:develop' into refactor-12189

9ea3368

[ISSUE alibaba#12189] refactor the server list manager code in the cl…

c0003e4

…ient module

KomachiSion requested changes Aug 1, 2024

View reviewed changes

misakacoder and others added 4 commits August 2, 2024 17:31

refactor the server list manager code in the client module

8d72924

Merge branch 'alibaba:develop' into refactor-12189

21cff2b

Merge branch 'refactor-12189' of https://github.com/misakacoder/nacos…

76f44fb

…-all into refactor-12189

refactor the server list manager code in the client module

551e699

KomachiSion requested changes Aug 5, 2024

View reviewed changes

misakacoder and others added 4 commits August 5, 2024 14:27

Merge branch 'alibaba:develop' into refactor-12189

c0fa8f2

refactor the server list manager code in the client module

47dbddc

refactor the server list manager code in the client module

3c42666

check style

850d3d5

test case

6c1468f

KomachiSion reviewed Aug 7, 2024

View reviewed changes

		@@ -52,7 +52,7 @@ public class PropertyKeyConst {
		public static final String RAM_ROLE_NAME = "ramRoleName";

		public static final String SERVER_ADDR = "serverAddr";

[ISSUE #12189] refactor the server list manager code in the client module #12366

Are you sure you want to change the base?

[ISSUE #12189] refactor the server list manager code in the client module #12366

Conversation

misakacoder commented Jul 17, 2024

What is the purpose of the change

Brief changelog

Verifying this change

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

KomachiSion commented Jul 18, 2024

KomachiSion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misakacoder commented Jul 19, 2024

misakacoder commented Jul 19, 2024

KomachiSion commented Jul 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KomachiSion commented Jul 23, 2024

KomachiSion commented Jul 29, 2024

misakacoder commented Jul 29, 2024

KomachiSion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misakacoder commented Aug 2, 2024

KomachiSion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misakacoder commented Aug 5, 2024

KomachiSion left a comment • edited Loading

Choose a reason for hiding this comment

misakacoder commented Aug 7, 2024

KomachiSion left a comment •

edited

Loading