You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by "1kasa (via GitHub)" <gi...@apache.org> on 2025/05/07 07:20:01 UTC
[PR] Multi registry verification and Use LoadOrStore instead [dubbo-go]
1kasa opened a new pull request, #2862:
URL: https://github.com/apache/dubbo-go/pull/2862
fix：https://github.com/apache/dubbo-go/issues/2094 Multi registry Address verification
Rewriting with LoadOrStore here can avoid the problem of duplicate creation under multi-threaded concurrency.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Multi registry verification and Use LoadOrStore instead [dubbo-go]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #2862:
URL: https://github.com/apache/dubbo-go/pull/2862#issuecomment-2865856512
## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=2862) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=2862&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=2862&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go&pullRequest=2862&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=2862&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=2862&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=2862)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Multi registry verification and Use LoadOrStore instead [dubbo-go]
Posted by "No-SilverBullet (via GitHub)" <gi...@apache.org>.
No-SilverBullet commented on PR #2862:
URL: https://github.com/apache/dubbo-go/pull/2862#issuecomment-2858392075
code looks good to me, have u ever validate this `registry verification` in your local environment?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Multi registry verification and Use LoadOrStore instead [dubbo-go]
Posted by "marsevilspirit (via GitHub)" <gi...@apache.org>.
marsevilspirit commented on PR #2862:
URL: https://github.com/apache/dubbo-go/pull/2862#issuecomment-2858533700
> > > It's like this. For example, multiple goroutines access a shared cache (such as proto.registries) at the same time, and these goroutines need to obtain the registry object based on the same cacheKey. If we use Load and Store, when multiple goroutines access it at the same time, it may happen that the `if !loaded {}` condition is executed, resulting in two identical Registry objects being stored in the cache, causing duplicate creation. After using LoadOrStore (which itself guarantees concurrent safety), no matter how many goroutines access it at the same time, as long as one goroutine successfully creates and stores the registry object, other goroutines will directly load the object from the cache, and duplicate creation will not occur.
> >
> >
> > Oh, I discovered a fatal problem. The config package is a package we are about to discard. We should implement this effect in global package.
>
> If this is the case,we may need to move the impact of this code to the global package module later.
I think so too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Multi registry verification and Use LoadOrStore instead [dubbo-go]
Posted by "1kasa (via GitHub)" <gi...@apache.org>.
1kasa commented on code in PR #2862:
URL: https://github.com/apache/dubbo-go/pull/2862#discussion_r2077292766
##########
config/root_config.go:
##########
@@ -367,3 +372,27 @@ func (rc *RootConfig) Process(event *config_center.ConfigChangeEvent) {
// dynamically update metric
rc.Metrics.DynamicUpdateProperties(updateRootConfig.Metrics)
}
+
+func validateRegistryAddresses(registries map[string]*RegistryConfig) error {
+ cacheKeyMap := make(map[string]string)
+
+ for id, reg := range registries {
+ address := reg.Address
+ namespace := reg.Namespace
+
+ cacheKey := address
+ if namespace != "" {
+ cacheKey = cacheKey + "?" + constant.NacosNamespaceID + "=" + namespace
+ }
+
+ if existingID, exists := cacheKeyMap[cacheKey]; exists {
+ err := fmt.Errorf("duplicate registry address: [%s] used by both [%s] and [%s]", cacheKey, existingID, id)
+ logger.Error(err)
Review Comment:
因为这里需要让调用者感知并处理错误，所以这里进行了定义并捕获err，如果只是单纯的打印可以直接使用logger.Errorf()来避免引入，但是这个函数是用来check的本身是不会有err定义并中断
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Multi registry verification and Use LoadOrStore instead [dubbo-go]
Posted by "1kasa (via GitHub)" <gi...@apache.org>.
1kasa commented on code in PR #2862:
URL: https://github.com/apache/dubbo-go/pull/2862#discussion_r2077285449
##########
config/root_config.go:
##########
@@ -367,3 +372,27 @@ func (rc *RootConfig) Process(event *config_center.ConfigChangeEvent) {
// dynamically update metric
rc.Metrics.DynamicUpdateProperties(updateRootConfig.Metrics)
}
+
+func validateRegistryAddresses(registries map[string]*RegistryConfig) error {
+ cacheKeyMap := make(map[string]string)
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Multi registry verification and Use LoadOrStore instead [dubbo-go]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #2862:
URL: https://github.com/apache/dubbo-go/pull/2862#issuecomment-2857397373
## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=2862) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=2862&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=2862&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go&pullRequest=2862&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=2862&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=2862&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=2862)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Multi registry verification and Use LoadOrStore instead [dubbo-go]
Posted by "marsevilspirit (via GitHub)" <gi...@apache.org>.
marsevilspirit commented on code in PR #2862:
URL: https://github.com/apache/dubbo-go/pull/2862#discussion_r2077231565
##########
config/root_config.go:
##########
@@ -367,3 +372,27 @@ func (rc *RootConfig) Process(event *config_center.ConfigChangeEvent) {
// dynamically update metric
rc.Metrics.DynamicUpdateProperties(updateRootConfig.Metrics)
}
+
+func validateRegistryAddresses(registries map[string]*RegistryConfig) error {
+ cacheKeyMap := make(map[string]string)
+
+ for id, reg := range registries {
+ address := reg.Address
+ namespace := reg.Namespace
+
+ cacheKey := address
+ if namespace != "" {
+ cacheKey = cacheKey + "?" + constant.NacosNamespaceID + "=" + namespace
+ }
+
+ if existingID, exists := cacheKeyMap[cacheKey]; exists {
+ err := fmt.Errorf("duplicate registry address: [%s] used by both [%s] and [%s]", cacheKey, existingID, id)
+ logger.Error(err)
Review Comment:
为什么不直接用logger.Errorf()?
这会导致多引入"fmt" package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Multi registry verification and Use LoadOrStore instead [dubbo-go]
Posted by "1kasa (via GitHub)" <gi...@apache.org>.
1kasa commented on PR #2862:
URL: https://github.com/apache/dubbo-go/pull/2862#issuecomment-2858511379
> code looks good to me, have u ever validate this `registry verification` in your local environment?
yes，I test local samples through the configuration file. If the address of the registry center in the configuration file (including namespace) conflicts, it will directly return an error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Multi registry verification and Use LoadOrStore instead [dubbo-go]
Posted by "1kasa (via GitHub)" <gi...@apache.org>.
1kasa commented on PR #2862:
URL: https://github.com/apache/dubbo-go/pull/2862#issuecomment-2858036091
It's like this. For example, multiple goroutines access a shared cache (such as proto.registries) at the same time, and these goroutines need to obtain the registry object based on the same cacheKey. If we use Load and Store, when multiple goroutines access it at the same time, it may happen that the `if !loaded {}` condition is executed, resulting in two identical Registry objects being stored in the cache, causing duplicate creation. After using LoadOrStore (which itself guarantees concurrent safety), no matter how many goroutines access it at the same time, as long as one goroutine successfully creates and stores the registry object, other goroutines will directly load the object from the cache, and duplicate creation will not occur.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Multi registry verification and Use LoadOrStore instead [dubbo-go]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #2862:
URL: https://github.com/apache/dubbo-go/pull/2862#issuecomment-2857963410
## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=2862) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=2862&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go&pullRequest=2862&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go&pullRequest=2862&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=2862&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go&pullRequest=2862&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go&pullRequest=2862)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Multi registry verification and Use LoadOrStore instead [dubbo-go]
Posted by "marsevilspirit (via GitHub)" <gi...@apache.org>.
marsevilspirit commented on PR #2862:
URL: https://github.com/apache/dubbo-go/pull/2862#issuecomment-2857993333
我想知道关于多线程并发导致重复注册的详细情况，你可以提供更多的信息吗，我们是否可以从根本解决问题？
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Multi registry verification and Use LoadOrStore instead [dubbo-go]
Posted by "No-SilverBullet (via GitHub)" <gi...@apache.org>.
No-SilverBullet merged PR #2862:
URL: https://github.com/apache/dubbo-go/pull/2862
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Multi registry verification and Use LoadOrStore instead [dubbo-go]
Posted by "marsevilspirit (via GitHub)" <gi...@apache.org>.
marsevilspirit commented on code in PR #2862:
URL: https://github.com/apache/dubbo-go/pull/2862#discussion_r2077296724
##########
config/root_config.go:
##########
@@ -367,3 +372,27 @@ func (rc *RootConfig) Process(event *config_center.ConfigChangeEvent) {
// dynamically update metric
rc.Metrics.DynamicUpdateProperties(updateRootConfig.Metrics)
}
+
+func validateRegistryAddresses(registries map[string]*RegistryConfig) error {
+ cacheKeyMap := make(map[string]string)
+
+ for id, reg := range registries {
+ address := reg.Address
+ namespace := reg.Namespace
+
+ cacheKey := address
+ if namespace != "" {
+ cacheKey = cacheKey + "?" + constant.NacosNamespaceID + "=" + namespace
+ }
+
+ if existingID, exists := cacheKeyMap[cacheKey]; exists {
+ err := fmt.Errorf("duplicate registry address: [%s] used by both [%s] and [%s]", cacheKey, existingID, id)
+ logger.Error(err)
Review Comment:
sounds good.
按你这个来吧。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Multi registry verification and Use LoadOrStore instead [dubbo-go]
Posted by "marsevilspirit (via GitHub)" <gi...@apache.org>.
marsevilspirit commented on PR #2862:
URL: https://github.com/apache/dubbo-go/pull/2862#issuecomment-2858393567
> It's like this. For example, multiple goroutines access a shared cache (such as proto.registries) at the same time, and these goroutines need to obtain the registry object based on the same cacheKey. If we use Load and Store, when multiple goroutines access it at the same time, it may happen that the `if !loaded {}` condition is executed, resulting in two identical Registry objects being stored in the cache, causing duplicate creation. After using LoadOrStore (which itself guarantees concurrent safety), no matter how many goroutines access it at the same time, as long as one goroutine successfully creates and stores the registry object, other goroutines will directly load the object from the cache, and duplicate creation will not occur.是这样的，比如多个 goroutine 同时访问一个共享缓存（比如 proto.registries），这些 goroutine 需要根据同一个 cacheKey 获取 registry 对象。如果我们使用 Load 和 Store，当多个 goroutine 同时访问时，
可能会发生 `if !loaded {}` 条件执行，导致两个相同的 Registry 对象被存储在缓存中，造成重复创建。而使用 LoadOrStore 之后（它本身就保证了并发安全），无论有多少个 goroutine 同时访问，只要有一个 goroutine 成功创建并存储了 registry 对象，其他 goroutine 就会直接从缓存中加载该对象，不会发生重复创建。
Oh, I discovered a fatal problem. The config is a package we are about to discard. We should implement this effect globally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Multi registry verification and Use LoadOrStore instead [dubbo-go]
Posted by "marsevilspirit (via GitHub)" <gi...@apache.org>.
marsevilspirit commented on code in PR #2862:
URL: https://github.com/apache/dubbo-go/pull/2862#discussion_r2077235600
##########
config/root_config.go:
##########
@@ -367,3 +372,27 @@ func (rc *RootConfig) Process(event *config_center.ConfigChangeEvent) {
// dynamically update metric
rc.Metrics.DynamicUpdateProperties(updateRootConfig.Metrics)
}
+
+func validateRegistryAddresses(registries map[string]*RegistryConfig) error {
+ cacheKeyMap := make(map[string]string)
Review Comment:
可以提前分配空间
cacheKeyMap := make(map[string]string, len(registries))
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Multi registry verification and Use LoadOrStore instead [dubbo-go]
Posted by "1kasa (via GitHub)" <gi...@apache.org>.
1kasa commented on PR #2862:
URL: https://github.com/apache/dubbo-go/pull/2862#issuecomment-2858527533
> > It's like this. For example, multiple goroutines access a shared cache (such as proto.registries) at the same time, and these goroutines need to obtain the registry object based on the same cacheKey. If we use Load and Store, when multiple goroutines access it at the same time, it may happen that the `if !loaded {}` condition is executed, resulting in two identical Registry objects being stored in the cache, causing duplicate creation. After using LoadOrStore (which itself guarantees concurrent safety), no matter how many goroutines access it at the same time, as long as one goroutine successfully creates and stores the registry object, other goroutines will directly load the object from the cache, and duplicate creation will not occur.
>
> Oh, I discovered a fatal problem. The config package is a package we are about to discard. We should implement this effect in global package.
If this is the case,we may need to move the impact of this code to the global package module later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org