[ISSUE #12189] Refactor Client SDK. #12432

XiaZhouxx · 2024-07-27T07:36:47Z

Please do not create a Pull Request without creating an issue first.

What is the purpose of the change

client sdk 寻址逻辑重构.

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.

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

CLAassistant · 2024-07-27T07:36:53Z

All committers have signed the CLA.

XiaZhouxx · 2024-07-27T07:37:56Z

第一次尝试提交PR, 希望大佬能够review指点一下, 非常感谢🙏

KomachiSion

看起来有不少冲突，您的PR基于的基础分支应该比较老旧了，建议尽快更新一下。

目前相对其他PR比较好的点：

保留了请求地址服务器时Header的语意。
通过ServerListHolder分离了Endpoint和ServerAddr的逻辑。

目前有问题的点：

配置中心ClientWorker和注册中心的HttpProxy的逻辑改动可能导致和以前的逻辑不一致，有风险
有些地址服务器相关参数过旧需要更新
地址服务器逻辑中缺少对queryParameter的支持（可以查看当前的配置中心部分的地址服务器寻址逻辑）

待确认：

同时传入endpoint和serverAddr，但从endpoint中获取到了返回码200，但是为空列表时，是否和现在的逻辑不一致

KomachiSion · 2024-07-29T02:19:58Z

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

-                            serverListMgr.getCurrentServerAddr(), result.getCode());
-                } else {
-                    // Update the currently available server addr
-                    serverListMgr.updateCurrentServerAddr(currentServerAddr);


这个逻辑是配置中心为了保持一个访问同一只可访问的server而保持的逻辑，我看你下面的acquireNextServer并没有保留这个逻辑，但是又移除了，可能有点问题。

这个逻辑是配置中心为了保持一个访问同一只可访问的server而保持的逻辑，我看你下面的acquireNextServer并没有保留这个逻辑，但是又移除了，可能有点问题。

这里是复用Naming模块的nextServer逻辑基于currentIndex, 在失败重试获取的逻辑里面会递增index, 应该不会导致Server下次访问会变动?

配置和服务的一致性协议有所区别，配置中心应该是尽可能保证访问到同一个节点上，注册中心可以随机分配。

因此这个逻辑之前时放在agent里和proxy里，没有放在地址服务器里。

既然这次是重构地址服务器，就不要改动外层agent和proxy的逻辑。

配置和服务的一致性协议有所区别，配置中心应该是尽可能保证访问到同一个节点上，注册中心可以随机分配。

因此这个逻辑之前时放在agent里和proxy里，没有放在地址服务器里。

既然这次是重构地址服务器，就不要改动外层agent和proxy的逻辑。

是的, 这里在重试genNextServer()找到可用Server后下标固定还是这个Server, 所以移除了updateCurrentServerAddr(), 注册中心则还是沿用以前的逻辑在构建时随机指定了一个下标。

KomachiSion · 2024-07-29T02:22:13Z

client/src/main/java/com/alibaba/nacos/client/constant/Constants.java

@@ -24,7 +24,10 @@
 * @author onew
 */
 public class Constants {
-
+    public static final String ENDPOINT_NAME = "endpoint";


为什么不复用com.alibaba.nacos.api.PropertyKeyConst？

为什么不复用com.alibaba.nacos.api.PropertyKeyConst？

这里是因为觉得有语意问题, 这个常量类都是用作NacosClientProperties在使用的.

KomachiSion · 2024-07-29T02:23:12Z

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

-                    if (NAMING_LOGGER.isDebugEnabled()) {
-                        NAMING_LOGGER.debug("request {} failed.", server, e);
-                    }
+        Random random = new Random();


有个问题，如果这样修改了，是否会导致如果只输入一个域名或ip的情况下，客户端失去了失败重试的能力。

KomachiSion · 2024-07-29T02:29:52Z

...ain/java/com/alibaba/nacos/client/serverlist/holder/impl/CompositeNacosServerListHolder.java

+                continue;
+            }
+            List<String> serverList = delegate.initServerList(properties);
+            if (CollectionUtils.isNotEmpty(serverList)) {


问个问题，如果我同时输入endpoint，和serverAddr，但是endpoint的地址返回的是空列表（内容为空列表，状态码为200），是否和原先的寻址逻辑不符合？

问个问题，如果我同时输入endpoint，和serverAddr，但是endpoint的地址返回的是空列表（内容为空列表，状态码为200），是否和原先的寻址逻辑不符合？

确实不符合, 我再优化一下, 只考虑重构没有考虑到兼容版本问题

KomachiSion · 2024-07-29T02:36:36Z

...main/java/com/alibaba/nacos/client/serverlist/holder/impl/EndpointNacosServerListHolder.java

+            if (!StringUtils.isBlank(contentPathTmp)) {
+                this.contentPath = contentPathTmp;
+            }
+            String serverListNameTmp = properties.getProperty(PropertyKeyConst.CLUSTER_NAME);


参数名已在2.3.3版本修改，请更新最新的代码。

参数名已在2.3.3版本修改，请更新最新的代码。

非常抱歉, 没有确认分支就开始开发, 我更新再优化一下

KomachiSion · 2024-07-29T02:36:52Z

...main/java/com/alibaba/nacos/client/serverlist/holder/impl/EndpointNacosServerListHolder.java

+        if (StringUtils.isNotEmpty(endpoint)) {
+            this.namespace = properties.getProperty(PropertyKeyConst.NAMESPACE);
+            this.moduleName = properties.getProperty(PropertyKeyConst.MODULE_NAME);
+            String contentPathTmp = properties.getProperty(PropertyKeyConst.CONTEXT_PATH);


CONTEXT_PATH的逻辑也有修改，请同步

# Conflicts: # client/src/main/java/com/alibaba/nacos/client/config/impl/ServerListManager.java # client/src/main/java/com/alibaba/nacos/client/naming/core/ServerListManager.java # client/src/test/java/com/alibaba/nacos/client/config/NacosConfigServiceTest.java # client/src/test/java/com/alibaba/nacos/client/config/http/ServerHttpAgentTest.java # client/src/test/java/com/alibaba/nacos/client/config/impl/ClientWorkerTest.java # client/src/test/java/com/alibaba/nacos/client/config/impl/ServerListManagerTest.java # client/src/test/java/com/alibaba/nacos/client/naming/remote/http/NamingHttpClientProxyTest.java # client/src/test/java/com/alibaba/nacos/client/naming/utils/ServerDiscoveryHttpUtilTest.java # client/src/test/java/com/alibaba/nacos/client/serverlist/ServerListManagerTest.java

XiaZhouxx · 2024-07-31T06:22:13Z

更新了一下, 也烦请大佬有空review一下

KomachiSion

感觉上问题已经快收敛完了，再看下评论进行一次修改。

另外有一个问题可以思考下：

moduleName时通过单独的参数传递进去还是方properties里比较好？
我感觉放properties有一点晦涩，直接传进去是不是明显一点。

KomachiSion · 2024-08-01T05:33:34Z

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

-                            serverListMgr.getCurrentServerAddr(), result.getCode());
-                } else {
-                    // Update the currently available server addr
-                    serverListMgr.updateCurrentServerAddr(currentServerAddr);


配置和服务的一致性协议有所区别，配置中心应该是尽可能保证访问到同一个节点上，注册中心可以随机分配。

因此这个逻辑之前时放在agent里和proxy里，没有放在地址服务器里。

既然这次是重构地址服务器，就不要改动外层agent和proxy的逻辑。

KomachiSion · 2024-08-01T05:35:30Z

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

        NacosException exception = new NacosException();
-


请使用nacos codestyle来reformat code，不要修改缩进

KomachiSion · 2024-08-01T05:42:38Z

client/src/main/java/com/alibaba/nacos/client/serverlist/ServerListManager.java

+        if (null != refreshServerListExecutor) {
+            ThreadUtils.shutdownThreadPool(refreshServerListExecutor, NAMING_LOGGER);
+        }
+        NamingHttpClientManager.getInstance().shutdown();


地址服务器应该处于更底层的逻辑，依赖NamingHttpClientManager可能不是很合适。
以及日志也打印在NAMING_LOGGER也不是很合适，

日志可以打印到REMOTE_LOGGER里， http访问可以通过NacosRestTemplate来替代。

地址服务器应该处于更底层的逻辑，依赖NamingHttpClientManager可能不是很合适。以及日志也打印在NAMING_LOGGER也不是很合适，

日志可以打印到REMOTE_LOGGER里， http访问可以通过NacosRestTemplate来替代。

是的, 作为底层逻辑不应该依赖上层业务我更正一下

KomachiSion · 2024-08-01T05:44:56Z

client/src/main/java/com/alibaba/nacos/client/serverlist/holder/NacosServerListHolders.java

+                    owner = delegate;
+                    return serverList;
+                }
+                throw new NacosLoadException("serverList is empty,please check configuration");


可以将NacosServerListHolder对应的class或name包含在异常中。这样提示信息能看到是目前是哪个类型被应用了，但是没获取到服务列表。

KomachiSion · 2024-08-01T05:46:34Z

console/src/main/resources/application.properties

-# db.password=nacos
+ db.url.0=jdbc:mysql://127.0.0.1:3306/nacos?characterEncoding=utf8&connectTimeout=1000&socketTimeout=3000&autoReconnect=true&useUnicode=true&useSSL=false&serverTimezone=UTC
+ db.user=root
+ db.password=118756


本地配置不用一起提交

XiaZhouxx · 2024-08-01T08:00:24Z

感觉上问题已经快收敛完了，再看下评论进行一次修改。

另外有一个问题可以思考下：

moduleName时通过单独的参数传递进去还是方properties里比较好？我感觉放properties有一点晦涩，直接传进去是不是明显一点。

确实, 看一下是有点晦涩，这个属性我觉得应该是必要属性要依赖方感知和设置这个属性, 放properties的话貌似还需要去理解上下文这个属性的作用, 假设另一个模块引用的话可能是不知道这个属性的.

XiaZhouxx · 2024-08-01T12:35:03Z

更正了一版, 对于ServerHttpAgent的改动之前有些问题这里修复了, 配置中心保持一个访问同一只可访问的server的逻辑是保留了的,在genNextServer()内部就已经修改当前服务下标并返回的, 之前是采用的迭代器方式才额外调用了方法修改当前服务. @KomachiSion 也麻烦大佬有空了看一下有什么问题

KomachiSion · 2024-08-05T01:44:47Z

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

-        boolean isFixServer = agent.serverListManager.isFixed;
-        metric.put("isFixedServer", isFixServer);
-        metric.put("addressUrl", agent.serverListManager.addressServerUrl);
+        metric.put("addressUrl", agent.serverListManager.getUrlString());


这个逻辑不一样吧，之前的这个记录的是地址服务器的信息，现在变成了获取到的地址。

KomachiSion · 2024-08-05T01:45:04Z

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

        metric.put("serverUrls", agent.serverListManager.getUrlString());
-
+        String serverListHolderName = agent.serverListManager.getServerListHolderName();
+        metric.put("isFixedServer", FixedConfigNacosServerListHolder.NAME.equals(serverListHolderName));


使用方还需要感知类型不太好。

KomachiSion · 2024-08-05T01:48:34Z

client/src/main/java/com/alibaba/nacos/client/naming/NacosNamingService.java

@@ -119,7 +118,7 @@ private void init(Properties properties) throws NacosException {
    private void initLogName(NacosClientProperties properties) {
        logName = properties.getProperty(UtilAndComs.NACOS_NAMING_LOG_NAME, DEFAULT_NAMING_LOG_FILE_PATH);
    }
-    
+


请勿修改缩进。

KomachiSion · 2024-08-05T01:51:21Z

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

@@ -372,7 +370,7 @@ public String reqApi(String api, Map<String, String> params, Map<String, String>
        } else {
            Random random = new Random();
            int index = random.nextInt(servers.size());
-            
+


缩进问题请全部检查一下

KomachiSion · 2024-08-05T01:54:42Z

client/src/main/java/com/alibaba/nacos/client/naming/utils/InitUtils.java

+     * @param properties nacos client properties
+     * @return
+     */
+    public static String initContextPath(NacosClientProperties properties) {


这个方法名最好叫initEndpointContextPath，不然之后的开发者可能会误解。

KomachiSion · 2024-08-05T03:01:18Z

@XiaZhouxx 您好，我收到主办方的消息，说您的PR没有在活动官网上登记，这样的话这个PR不算是有效的参赛PR，

方便抽空把报名信息在官网上填写一下。

https://tianchi.aliyun.com/competition/entrance/532215/information?spm=a2c22.12281976.0.0.41ab6c71nLISOm

对填写有疑问也可以发邮件到 [email protected] 询问。

XiaZhouxx · 2024-08-05T04:22:53Z

@XiaZhouxx 您好，我收到主办方的消息，说您的PR没有在活动官网上登记，这样的话这个PR不算是有效的参赛PR，

方便抽空把报名信息在官网上填写一下。

https://tianchi.aliyun.com/competition/entrance/532215/information?spm=a2c22.12281976.0.0.41ab6c71nLISOm

对填写有疑问也可以发邮件到 [email protected] 询问。

收到，已经填报PR, 代码也重新格式化排版 @KomachiSion 也烦请大佬抽空看一下

KomachiSion

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

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

XiaZhouxx · 2024-08-07T07:35:02Z

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

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

好的, 辛苦了

XiaZhouxx · 2024-08-07T10:11:29Z

@KomachiSion 抱歉大佬有几个文件没有添加licenses, 麻烦再跑下呢

XiaZhouxx added 3 commits December 1, 2023 16:04

register

5ced724

[ISSUE alibaba#12189]Client Refactor.

1ed3f73

优化client逻辑

b0ea4df

KomachiSion added the Summer label Jul 29, 2024

KomachiSion requested changes Jul 29, 2024

View reviewed changes

XiaZhouxx added 2 commits July 29, 2024 15:59

client server list refactor.

d9be6af

KomachiSion requested changes Aug 1, 2024

View reviewed changes

XiaZhouxx added 2 commits August 1, 2024 20:07

优化并剔除冲突

0b7e4be

优化并剔除冲突

aa4bab2

XiaZhouxx added 2 commits August 4, 2024 22:14

保留重构前指标

ae4bcb9

保留重构前指标

bd3a2cf

KomachiSion requested changes Aug 5, 2024

View reviewed changes

XiaZhouxx added 2 commits August 5, 2024 11:57

format code style.

ff1411f

保留 ClientWorker 指标逻辑.

bc4f936

XiaZhouxx added 3 commits August 5, 2024 14:02

format code style.

5ddaf34

修复测试用例问题.

b5d68b2

修复测试用例以及代码缩进.

747f673

KomachiSion reviewed Aug 7, 2024

View reviewed changes

add license.

d9f5e2e

KomachiSion mentioned this pull request Aug 27, 2024

用通义灵码，人人都是开源贡献者：利用通义灵码，帮助Nacos Client统一寻址模块的代码，并提供自定义拓展能力。 #12189

Open

[ISSUE #12189] Refactor Client SDK. #12432

Are you sure you want to change the base?

[ISSUE #12189] Refactor Client SDK. #12432

Conversation

XiaZhouxx commented Jul 27, 2024

What is the purpose of the change

Brief changelog

Verifying this change

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

CLAassistant commented Jul 27, 2024 • edited Loading

XiaZhouxx commented Jul 27, 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

XiaZhouxx commented Jul 31, 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

XiaZhouxx commented Aug 1, 2024

XiaZhouxx commented Aug 1, 2024

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

KomachiSion commented Aug 5, 2024 • edited Loading

XiaZhouxx commented Aug 5, 2024

KomachiSion left a comment

Choose a reason for hiding this comment

XiaZhouxx commented Aug 7, 2024

XiaZhouxx commented Aug 7, 2024

CLAassistant commented Jul 27, 2024 •

edited

Loading

KomachiSion commented Aug 5, 2024 •

edited

Loading