alibaba / nacos

an easy-to-use dynamic service discovery, configuration and service management platform for building cloud native applications.
https://nacos.io
Apache License 2.0
30.06k stars 12.8k forks source link

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

Open KomachiSion opened 3 months ago

KomachiSion commented 3 months ago

天池大赛 - 用通义灵码,人人都是开源贡献者

今年的天池大赛中将举办一个特殊的比赛,你可以借助通义灵码的检索增强能力对开源代码进行智能发现代码优化方向,输出 PR 或者根据开源社区诉求,开发新功能,提交专属 PR。更多赛事介绍请查看大赛详情

Nacos社区作为被邀请的开源项目,将会选择一个社区任务作为比赛的课题,提供给参赛选手进行攻克,考虑到通义灵码的能力,最后社区选择的课题为:利用通义灵码,帮助Nacos Client统一寻址模块的代码,并提供自定义拓展能力

利用通义灵码,帮助Nacos Client统一寻址模块的代码,并提供自定义拓展能力。

该课题衍生自课题ISSUE#8310通过插件方式统一注册中心和配置中心的寻址模块。

虽然在之前的活动中已经尝试对该课题进行解决和处理,但当时的目标不仅需要统一客户端中注册中心和配置中心的寻址模块,还需要将客户端和服务端的寻址模块一起进行统一,且需要以插件的形式实现。随着项目的开发进展以及实践的反馈中,社区发现客户端和服务端的寻址模块在诸多方面有着不同,比如在对一致性的要求、在可用性的要求上。 因此社区的主线分支并没有采纳这种方案。

不过,虽然客户端和服务端之间的寻址模块诉求不同, 但是客户端中注册中心和配置中心的寻址诉求确实相同的,而目前客户端中注册中心和配置中心的寻址模块功能高度重合却又代码独立,这导致有时同一个问题需要在两个地方同时修复,容易造成遗漏,同时对于代码简洁性和复用度上都有较大欠缺。如ISSUE#9824 所提出的问题。

因此社区希望,选手通过通义灵码的代码解释及上下文对比的能力,对注册中心和配置中心的寻址模块的代码进行比对,将其公共部分的逻辑抽象出来,单独作为共用的寻址模块,同时设计一个可拓展的API,以提供用户能够添加更多类型的寻址方式。

赛事面向对象

成果要求

获胜条件

提出的方案最终被社区采纳,且提出的PR被社区所合并,目前活动规则仅一人的方案和PR会被最终采纳。

活动时间

2024年6月20日-2024年8月22日

The-Gamer-01 commented 3 months ago

支持支持

KomachiSion commented 2 months ago

https://github.com/alibaba/nacos/issues/12218

后面参与课题的同学可以多考虑一下上面这个issue的问题。并随时关注这个issue中的结论。

KomachiSion commented 2 months ago

根据历史版本和多数使用场景的行为,发现绝大多数情况和场景下,都是endpoint优先, 仅在配置中心的parsing=false时优先,且分析应该目前没有parsing=false且同时配置endpoint和serverAddr的诉求。

因此最终预期,统一成endpoint优先:

  1. parsing=true(默认)情况下,保持现状,即:最 高优先级环境变变量 ALIBABA_ALIWARE_ENDPOINT_URL -> 其次是 用户传入的endpoing-Dendpoint -> 最后是传入的serverAddr-DserverAddr.
  2. parsing=false情况下, 忽略ALIBABA_ALIWARE_ENDPOINT_URL, 其他保持和parsing=true一致, 即 用户传入的endpoing-Dendpoint -> 传入的serverAddr-DserverAddr.

后续做课题的同学,尽量保持现有的寻址逻辑和优先级顺序, 但是建议能够对ConfigService所对应的ServerListManager的优先级顺序进行调整不强制,仅作为加分项

最终影响是否被采纳和合入的还是 重构的准确性代码质量以及对更多寻址能力的可拓展能力

KomachiSion commented 3 weeks ago

《天池大赛 - 用通义灵码,人人都是开源贡献者》活动已进入尾声,进入评分阶段。参赛PR基本满足ISSUE要求的条件,达到了可以合并的要求,经过社区导师和专家的评审,针对参赛PR进行评价如下:

PR提交者基本都采用了将服务地址管理ServerListManager和服务地址提供ServerListProvider拆分的方案进行最终开发,在大体方案上接近的情况下,评审主要针对功能的完整度、代码的可读性、面向失败的情况处理等因素进行打分。

--- #12274

该参赛者为第一个提交的PR,从提交历史来看,也是首个提交Provider和Manager拆分的参赛者

  1. PR较好的设计和实现

    • 将获取server list(ServerListProvider)逻辑和管理server list(ServerListManager)的逻辑拆分到不同的抽象中解耦,并允许用户通过SPI扩展获取server list的逻辑。
    • 充分考虑当前注册中心模块和配置中心模块的差异,支持不同模块拓展不同ServerListManager实现。
    • 异步从地址服务器更新地址的线程内聚到地址服务器的相关实现中,通过事件通知的方式反馈管理器和外部监听事件需求的地方进行更新。
    • 对RequestHeader的差异处理的比较好。
  2. PR设计和实现上的问题

    • 虽然拆分了ServerListProvider和ServerListManager,但是还是将ServerListProvider暴露出去(getServerListProvider),使得执行不够彻底,不够内聚,让ServerListManager更像是ServerListProviderManager。
    • ServerListProviderOrder看上去是希望将Order作为一个逻辑来实现,但是最终只是一个常量的存储,显得比较多余,其实直接在Constants里添加常量即可。
    • ServerListManager在构造方法中直接抛出异常,这应该是尽量避免的设计。
    • 未对SPI的实现做异常处理,可能导致其中一个错误的SPI实现抛出异常影响到其他SPI的加载以及整个Client的初始化。
    • 未考虑地址服务器返回的数据顺序问题,如1.1.1.1,2.2.2.22.2.2.2,1.1.1.1可能被识别为不同的数据。

评分: 完成度16+可读性12+创新分15=43

--- #12366

该参赛者提供的内容最为丰富,但很多修改超出了本ISSUE所要求的范围,因此暂时要求移除,等待后续再贡献社区。

  1. PR较好的设计和实现

    • 将获取server list(ServerListProvider)逻辑和管理server list(ServerListManager)的逻辑拆分到不同的抽象中解耦,并允许用户通过SPI扩展获取server list的逻辑。
    • 充分考虑当前注册中心模块和配置中心模块的差异,支持不同模块拓展不同ServerListManager实现。
    • 有考虑到地址服务器返回的数据顺序问题,如1.1.1.1,2.2.2.22.2.2.2,1.1.1.1会被统一排序,认为是同样的地址。
    • ServerListManager可根据ServerListProvider是否支持更新,判断是否启动异步线程来统一触发地址更新,并统一通过事件通知。
  2. PR设计和实现上的问题

    • 未对SPI的实现做异常处理,可能导致其中一个错误的SPI实现抛出异常影响到其他SPI的加载以及整个Client的初始化。
    • 虽然在ServerListManager中将初始化的部分方法抽象成为单独的方法,但是还是在构造方法中直接抛出异常,这应该是尽量避免的设计。
    • 初始化ServerListManager时,获取Provider的逻辑复杂度略高,在一个for循环中层层if深入,可以优化成先获取Provider,然后再用provider获取地址,最后判断是否支持更新来启动异步线程。
    • Provider的部分方法名定义不合理,比如isValid换成isEnabled可能更贴切,再比如getAddressServerUrl过于明确,甚至接口注释中还写明了Only for AddressServerListProvider,不像定义的接口。
    • 对RequestHeader的差异处理的比较比较生硬, 在地址服务器实现逻辑中固定判断是否为Naming模块。

评分:完成度17+可读性11+创新分13=41

--- #12432

  1. PR较好的设计和实现

    • 将获取server list(ServerListHolder)逻辑和管理server list(ServerListManager)的逻辑拆分到不同的抽象中解耦,并允许用户通过SPI扩展获取server list的逻辑。
    • 对RequestHeader的差异处理的比较好。
  2. PR设计和实现上的问题

    • 只使用一个统一ServerListManager承载所有共有逻辑,使得部分特殊逻辑需要下沉到使用处,导致ServerHttpAgent对应逻辑的改动,略微不符合重构的原则。
    • ServerListHolder的命名相较于另外两个PR不是那么准确。
    • 未对ServerListHolder判断是否支持更新地址,均创建了线程自动更新。
    • ServerListManager在构造方法中直接抛出异常,这应该是尽量避免的设计。
    • 未对SPI的实现做异常处理,可能导致其中一个错误的SPI实现抛出异常影响到其他SPI的加载以及整个Client的初始化。
    • 未考虑地址服务器返回的数据顺序问题,如1.1.1.1,2.2.2.22.2.2.2,1.1.1.1可能被识别为不同的数据。

    评分:完成度13+可读性10+创新分10=33

总结:

虽然大家都采用了服务地址管理ServerListManager和服务地址提供ServerListProvider拆分的方案, 但是大家在具体的实现和接口抽象上各有千秋,同时大家对于SPI加载时的异常处理和构造方法中的异常处理都没有进行考虑; 同时,大家对于需要异步线程来更新的地址服务器模式的处理方式也不相同,有的是通过实现的接口来判断是否需要、有的是下沉到实现内部自行管理、也有不论是否需要,都创建固定线程池来定期处理的,其实三种方案都没有问题,但是从合理性来看,方案二应该最为准确合理(ServerListManager管理ServerList的生命周期,ServerListProvider只负责提供地址),因此第二个PR的完成度最高; 从代码的可读性来看,第一个PR的代码读起来歧义最小,基本通读下来不会造成太大的误解,改动也比较小,只是有一些命名上还可以再进行优化,第二个PR也很出色,但是相对第一个略微差一些,第三个PR的则不够内聚,导致了外层使用区域的代码改动,相对来说最不理想; 因为最终大家都选择了服务地址管理ServerListManager和服务地址提供ServerListProvider拆分的方案,因此给予第一个提交PR且直接采用了该设计方案的第一个PR更多的创新分。

XiaZhouxx commented 3 weeks ago

非常感谢大佬指出的问题,第一次参与开源项目PR让我认识到自身还有很多思考量不足,后续会继续学习,争取能为社区贡献。

totalo commented 3 weeks ago

感谢大佬,在这个过程中学习到很多,辛苦啦

misakacoder commented 2 weeks ago

感谢大佬点评,在此过程中学习到了不少知识,非常感谢