Skip to content
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

Fix:nacos client reuse #1633

Merged
merged 16 commits into from
Feb 1, 2022
Merged

Fix:nacos client reuse #1633

merged 16 commits into from
Feb 1, 2022

Conversation

zhaoyunxing92
Copy link
Contributor

What this PR does:
目前创建nacos客户端的时候出现复用链接问题,会导致配置文件最终只有一个生效
Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


@zhaoyunxing92 zhaoyunxing92 added the bug Something isn't working label Dec 2, 2021
@zhaoyunxing92 zhaoyunxing92 added this to the v3.0.0 milestone Dec 2, 2021
@LaurenceLiZhixin LaurenceLiZhixin removed this from the v3.0.0 milestone Dec 5, 2021
@LaurenceLiZhixin
Copy link
Contributor

本地跑下单元测试,应该是有问题的

@zhaoyunxing92 zhaoyunxing92 changed the title [WIP]Fix:nacos client reuse Fix:nacos client reuse Jan 10, 2022
@LaurenceLiZhixin
Copy link
Contributor

@zhaoyunxing92 You can merge 3.0 and try again.

@AlexStocks
Copy link
Contributor

cc @sanxun0325

@@ -69,6 +69,11 @@ func (CenterConfig) Prefix() string {
return constant.ConfigCenterPrefix
}

// NameId unique identifier id for client
func (c *CenterConfig) NameId() string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may use ID instead of Id.

// NewNacosConfigClientByUrl read the config from url and build an instance
func NewNacosConfigClientByUrl(url *common.URL) (*nacosClient.NacosConfigClient, error) {
sc, cc, err := GetNacosConfig(url)
if err != nil {
return nil, err
}
return nacosClient.NewNacosConfigClient(nacosClientName, true, sc, cc)
clientName := url.GetParam(constant.ClientNameKey, "")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

按照这个pr的实现理论上应该都可以在url获取到clientName,如果没有获取到是不是默认生成一下nameID更好,不抛出错误。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我理解应该需要强制抛异常,使用默认又会导致全局只有一个client配置

@@ -69,6 +69,11 @@ func (CenterConfig) Prefix() string {
return constant.ConfigCenterPrefix
}

// NameId unique identifier id for client
func (c *CenterConfig) NameId() string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NameId func 好几个文件都有实现,可以提取到一个公共函数中么,接收实现的Prefix的interface就好

@AlexStocks AlexStocks merged commit 8041ace into apache:3.0 Feb 1, 2022
@binbin0325 binbin0325 mentioned this pull request Feb 12, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants