nacos icon indicating copy to clipboard operation
nacos copied to clipboard

[ISSUE #12189] Refactor Client SDK.

Open XiaZhouxx opened this issue 1 year ago • 9 comments

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.

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

image image

XiaZhouxx avatar Jul 27 '24 07:07 XiaZhouxx

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Jul 27 '24 07:07 CLAassistant

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

XiaZhouxx avatar Jul 27 '24 07:07 XiaZhouxx

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

XiaZhouxx avatar Jul 31 '24 06:07 XiaZhouxx

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

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

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

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

XiaZhouxx avatar Aug 01 '24 08:08 XiaZhouxx

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

XiaZhouxx avatar Aug 01 '24 12:08 XiaZhouxx

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

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

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

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

KomachiSion avatar Aug 05 '24 03:08 KomachiSion

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

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

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

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

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

XiaZhouxx avatar Aug 05 '24 04:08 XiaZhouxx

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

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

好的, 辛苦了

XiaZhouxx avatar Aug 07 '24 07:08 XiaZhouxx

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

XiaZhouxx avatar Aug 07 '24 10:08 XiaZhouxx

根据最后的评分结果, https://github.com/alibaba/nacos/pull/12274 会被合并入主干分支,

感谢参赛同学的贡献,该PR也有很多可取之处, 欢迎继续贡献此模块内容,将这部分内容优化的更好。

KomachiSion avatar Oct 15 '24 02:10 KomachiSion