Copilot commented on code in PR #3635:
URL: https://github.com/apache/dubbo-go/pull/3635#discussion_r3755095973
##########
remoting/nacos/builder.go:
##########
@@ -43,6 +43,23 @@ var (
newNacosConfigClient = nacosClient.NewNacosConfigClient
)
+// nacosClientPoolKey derives the gost client-pool key from the fields that
+// distinguish one nacos connection from another: endpoint/address, namespace
+// and credentials. Components pointing at the same cluster (registry,
+// config-center, metadata-report) resolve to the same key and share one SDK
+// client session instead of each opening its own. Role-scoped client names
+// must not be used as the key — they would defeat the sharing.
+func nacosClientPoolKey(kind string, url *common.URL) string {
+ return strings.Join([]string{
+ "dubbo-nacos", kind,
+ url.GetParam(constant.NacosEndpoint, ""),
+ url.Location,
+ url.GetParam(constant.NacosNamespaceID, ""),
+ url.GetParam(constant.NacosUsername, ""),
+ url.GetParam(constant.NacosAccessKey, ""),
+ }, "|")
+}
Review Comment:
nacosClientPoolKey currently includes both NacosEndpoint and url.Location in
the key, but GetNacosConfig ignores url.Location when NacosEndpoint is set.
That means two URLs pointing at the same endpoint can fail to share a client
because their (ignored) Location differs. Also, the key currently embeds
credential fields (username/accessKey) directly, which risks leaking secrets if
the pool key is ever logged or exported; it’s safer to include a stable
fingerprint of the credential set rather than raw values.
##########
metadata/report/zookeeper/report.go:
##########
@@ -213,10 +213,14 @@ type zookeeperMetadataReportFactory struct{}
// CreateMetadataReport creates the zookeeper-based metadata report
implementation.
func (mf *zookeeperMetadataReportFactory) CreateMetadataReport(url
*common.URL) report.MetadataReport {
+ // Join the gost shared-client pool under the same key (url.Location)
that
+ // registry and config-center use via ValidateZookeeperClient, so all
roles
+ // pointing at the same cluster reuse one ZooKeeper session. The pool is
+ // reference-counted: Close only disconnects when the last user is gone.
client, err := gxzookeeper.NewZookeeperClient(
- "zookeeperMetadataReport",
+ url.Location,
strings.Split(url.Location, ","),
- false,
+ true,
gxzookeeper.WithZkTimeOut(url.GetParamDuration(constant.TimeoutKey, "15s")),
Review Comment:
Now that metadata-report joins the shared ZooKeeper client pool (share=true,
key=url.Location), the client’s timeout may be taken from whichever role
creates the pooled client first. ValidateZookeeperClient uses ConfigTimeoutKey
with DefaultRegTimeout, but metadata-report currently uses TimeoutKey with a
different default (15s), making the shared-client timeout dependent on creation
order and parameter naming. Consider preferring ConfigTimeoutKey and falling
back to TimeoutKey for compatibility.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
