-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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] refactor the server list manager code in the client module #12366
base: develop
Are you sure you want to change the base?
Conversation
已收到PR, 会尽快进行review,请时刻关注 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
方案中可以认可的点:
- 用SPI进行扩展。
- 将AddressServer和Properties独立作为两个拓展实现。
- 将公共逻辑抽象到抽象类中
可能有问题的点:
- SPI不需要自己实现,我理解jdk自带的足以支持
- order的排序方式可以写入到AbstractServerListManager,作为方法提供,用注解 反射的方法可能没那么好。
- 事件不需要修改名称, 旧的名称足以表达意思
- 地址服务器的实现可能有逻辑遗漏,建议在梳理一下,做好注册和配置中心的区分(命名和调用逻辑上。
@@ -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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个Server file是什么?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个Server file是什么?
这个是我参考的MemberLookup的实现逻辑,多加了一个,如果觉得暂时没有必要可以去掉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
先去掉吧, 本issue和pr以重构和留出拓展接口为主, 如果想把这个实现贡献到社区, 可以考虑这个PR合并之后,再添加一个issue和PR来支持。
try { | ||
String managerName = cls.getSimpleName(); | ||
Constructor<AbstractServerListManager> constructor = cls.getConstructor(NacosClientProperties.class); | ||
AbstractServerListManager serverListManager = constructor.newInstance(clientProperties); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如果用SPI的话,最好使用无参数构造器, 然后调用init方法的方式处理,否则需要构建出两次实例。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如果用SPI的话,最好使用无参数构造器, 然后调用init方法的方式处理,否则需要构建出两次实例。
就是jdk的SPI不支持有参构造,和会自动实例化类,我才手动实现了一个,你看看嘛,你觉得没有必要我就先去掉
|
||
private static int getOrder(Class<AbstractServerListManager> cls) { | ||
Order order = cls.getAnnotation(Order.class); | ||
return order != null ? order.value() : Integer.MAX_VALUE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
同样不建议用反射在获取。可以使用getOrder方法替代。
comparator = comparator != null ? comparator : defaultComparator; | ||
List<Class<AbstractServerListManager>> classes = ServiceLoader.load(AbstractServerListManager.class); | ||
classes.sort(comparator); | ||
for (Class<AbstractServerListManager> cls : classes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这种循环的话,好像和旧的逻辑会有区别。
之前的逻辑,如果endpoint不为空,但是从endpoint的地址服务器上获取到空列表的话,会直接报错serverlist是empty。
但是这个逻辑似乎会出现去使用serverAddr的值的情况。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这种循环的话,好像和旧的逻辑会有区别。
之前的逻辑,如果endpoint不为空,但是从endpoint的地址服务器上获取到空列表的话,会直接报错serverlist是empty。
但是这个逻辑似乎会出现去使用serverAddr的值的情况。
我之前考虑的是按顺序搜寻可用的nacos地址,如果最终所有寻址方式都不能搜寻到可用的nacos服务,就抛异常。
如果和之前保持一直的话,我这样实现,如果每种寻址方式的特定配置,比如endpoint,serverAddr配置了,但是没有获取到nacos服务列表,就直接抛出异常,这样怎么样呢
*/ | ||
public class ServerListChangeEvent extends SlowEvent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
不用修改事件名吧
* | ||
* @author misakacoder | ||
*/ | ||
public class ServiceLoader { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
不需要自己实现ServiceLoader, java本身有实现。 如果jdk本身的不符合需求, 也有其他社区优秀的实现,避免重复造轮子。
} | ||
|
||
@Override | ||
protected String initServerName(NacosClientProperties properties) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如何区分是注册中心模块还是配置中心模块呢?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如何区分是注册中心模块还是配置中心模块呢?
原来的代码,只有配置中心的ServerListManager才有initServerName方法,最终暴露出去的是getName方法,作用是生成FailoverFile和SnapshotFile,注册中心没有这个哦
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
但是重构之后,注册和配置中心都会复用这个name的代码,可能会生成出一样的名字。
|
||
private List<String> readServerList() { | ||
try { | ||
HttpRestResult<String> result = nacosRestTemplate.get(addressServerUrl, Header.EMPTY, Query.EMPTY, String.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里会丢失注册中心和配置中心的区别。 注册中心有一个header
Header header = NamingHttpUtil.builderHeader();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里会丢失注册中心和配置中心的区别。 注册中心有一个header
Header header = NamingHttpUtil.builderHeader();
这个具体看下我下面的回复,我理解的是注册中心和配置中心是一样的,就是请求url,获取响应得到nacos服务列表
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个Header很重要, 里面会带一个Request-Module的信息,用于区分需要注册中心的地址还是配置中心的。
* @author misakacoder | ||
*/ | ||
@Order(200) | ||
public class FileServerListManager extends AbstractServerListManager { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
添加新增的寻址扩展方法,可以在后续增加, PR还是聚焦在重构上。
|
||
/** | ||
* Server Agent. | ||
* | ||
* @author water.lyl | ||
*/ | ||
public class ServerHttpAgent implements HttpAgent { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
请使用nacos code style进行reformat,不需要修改缩进。
@KomachiSion 幸苦空了看看我的回复和疑惑,我这边好调整调整 |
@@ -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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
先去掉吧, 本issue和pr以重构和留出拓展接口为主, 如果想把这个实现贡献到社区, 可以考虑这个PR合并之后,再添加一个issue和PR来支持。
} | ||
|
||
@Override | ||
protected String initServerName(NacosClientProperties properties) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
但是重构之后,注册和配置中心都会复用这个name的代码,可能会生成出一样的名字。
|
||
private List<String> readServerList() { | ||
try { | ||
HttpRestResult<String> result = nacosRestTemplate.get(addressServerUrl, Header.EMPTY, Query.EMPTY, String.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个Header很重要, 里面会带一个Request-Module的信息,用于区分需要注册中心的地址还是配置中心的。
辛苦再调整一下。 |
@misakacoder any update? |
不好意思,前几天有点忙,我调整了下,幸苦帮忙review下呢 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
辛苦再看下评论,进行一下修改。
大体上没有问题了。一些细节问题处理一下。
@@ -52,7 52,7 @@ public class PropertyKeyConst { | |||
public static final String RAM_ROLE_NAME = "ramRoleName"; | |||
|
|||
public static final String SERVER_ADDR = "serverAddr"; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
请使用nacos codeStyle进行格式化, 无需修改缩进
String providerName = provider.getClass().getSimpleName(); | ||
provider.startup(properties, namespace, getModuleType()); | ||
List<String> serverList = provider.getServerList(); | ||
if (CollectionUtils.isNotEmpty(serverList)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个地方我好像评论过, 如果只以是否为空的情况判断,可能会造成不符合预期的情况:
endpoint和serverAddr都传递,但是从endpoint处获取到地址列表为空时,旧版本是抛出错误,认为当前没有可用地址,但是心逻辑是用了serverAddr。
看下是有有个接口,比如isApplied或者类似的, 表达这个provider在当前配置下是否应该使用,以此来决定是否使用这个provider,而不是通过serverlist来判断。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
其他部分逻辑应该比较完善了
} | ||
|
||
private void updateServerList(List<String> serverList) { | ||
if (serverList == null || serverList.isEmpty() || serverList.equals(this.serverList)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里我确认一下, this.serverList经过repairServerAddr之后,和原本的list可能会不一致,会不会导致不停的更新的情况。
另外这里注意一下顺序问题, 比如1.1.1.1,2.2.2.2.和 2.2.2.2,1.1.1.1 是否需要更新。
boolean isFixServer = agent.serverListManager.isFixed; | ||
AbstractServerListManager serverListManager = agent.serverListManager; | ||
ServerListProvider serverListProvider = serverListManager.getServerListProvider(); | ||
boolean isFixServer = serverListProvider instanceof PropertiesServerListProvider; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里也是建议将这个属性抽象成serverListProvider的方法, 比如isFixedServer 或者 isDynamicServer 等能表达类似意思的。
否则这里使用处还要感知具体类型,不是很好。
当然可以从AbstractServerListManager暴露也行,AbstractServerListManager不是有判断是否需要构造线程池来更新地址吗?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KomachiSion 大佬,我优化了一下,麻烦再review一下呢
metric.put("serverUrls", agent.serverListManager.getUrlString()); | ||
if (serverListProvider instanceof AddressServerListProvider) { | ||
metric.put("addressUrl", ReflectUtils.getFieldValue(serverListProvider, "addressServerUrl")); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
同上, 可以配合上面那个接口,一起处理。
NAMING_LOGGER.debug("request {} failed.", nacosDomain, e); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这段逻辑的移除,会不会导致仅有一个地址的时候不进行重试了?
@KomachiSion 大佬,我优化了一下,麻烦再review一下呢 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
两个建议:
- 关于ServerHttpAgent文件的改动, 我觉得最好不要进行大改,保持原有逻辑,因为我们是寻址模块的重构,而不是Agent的重构。如果有兴趣,可以在这个PR结束之后,单独提交。
- Provider的startup其实我建议在valid之后,因为地址服务器以及可能后续的用户自定义Provider, 都可能在startup中有网络或IO操作,这可能导致耗时以及报错异常等,如果放在valid之前,花费时间以及内存进行了多项初始化工作,甚至是IO操作,然后不满足valid条件被忽略, 有点得不偿失。
另外 关于NamingHttpClientProxy丢失的逻辑,依然没有补回。
public String getAddressServerUrl() { | ||
String addressServerUrl = null; | ||
if (serverListProvider instanceof AddressServerListProvider) { | ||
addressServerUrl = ReflectUtils.getFieldValue(serverListProvider, "addressServerUrl").toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
用反射是不是不太好?
是不是改为provider提供一个getRemoteProviderUrl方法来获取比较好, 默认这个方法可以返回一个空字符串,或者返回一个Optional.
@KomachiSion 已修改,麻烦再review下呢,另外NamingHttpClientProxy只有一个地址就不重试的逻辑我是优化成这样了。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
目前初步review似乎没有太大的问题了, 之后会在活动截止前对PR进行整体评判后决定是否合入。
好的,感谢 |
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:
[ISSUE #123] Fix UnknownException when host config not exist
. Each commit in the pull request should have a meaningful subject line and body.mvn -B clean package apache-rat:check findbugs:findbugs -Dmaven.test.skip=true
to make sure basic checks pass. Runmvn clean install -DskipITs
to make sure unit-test pass. Runmvn clean test-compile failsafe:integration-test
to make sure integration-test pass.本改动借助了通义灵码进行辅助编程