You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by "somiljain2006 (via GitHub)" <gi...@apache.org> on 2025/12/21 11:27:46 UTC
[PR] Reuse Nacos NamingService connections across registry groups [dubbo]
somiljain2006 opened a new pull request, #15891:
URL: https://github.com/apache/dubbo/pull/15891
## What is the purpose of the change?
This change prevents the creation of multiple physical Nacos NamingService connections when a single Dubbo application uses multiple registry groups pointing to the same Nacos server. It introduces a reference-counted cache that reuses a single NamingService instance per unique Nacos connection identity, ensuring connections are only closed when no registries remain.
Fixes #15624
## Checklist
- [x] Make sure there is a [GitHub_issue](https://github.com/apache/dubbo/issues) field for the change.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Write necessary unit tests to verify your logic correction. If the new feature or significant change is committed, please remember to add a sample in the [dubbo samples](https://github.com/apache/dubbo-samples) project.
- [x] Make sure GitHub actions can pass. [Why the workflow is failing and how to fix it?](../CONTRIBUTING.md)
--
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] Reuse Nacos NamingService connections across registry groups [dubbo]
Posted by "RainYuY (via GitHub)" <gi...@apache.org>.
RainYuY commented on PR #15891:
URL: https://github.com/apache/dubbo/pull/15891#issuecomment-3681998104
Hmm. I’m wondering if we just remove the call to addParameter(CONFIG_NAMESPACE_KEY) from org.apache.dubbo.registry.nacos.NacosRegistryFactory#createRegistryCacheKey, will we get the same result?
--
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] Reuse Nacos NamingService connections across registry groups [dubbo]
Posted by "RainYuY (via GitHub)" <gi...@apache.org>.
RainYuY commented on PR #15891:
URL: https://github.com/apache/dubbo/pull/15891#issuecomment-3682173053
> > No, that would cause different namespaces to collapse into the same cache key. Also, relying solely on RegistryFactory caching is unsafe when multiple services share a Registry instance; one service calling destroy() would physically kill the connection for everyone else. We need the reference counting to manage that lifecycle.
>
> Regarding the first point, I don’t know what problems the same cache key will cause. In fact, I think the same cache key should yield the correct result—maybe there’s some reason behind the special logic. However, I don’t think the second scenario exists. Because as for the registry, I believe we only destroy it when destroying all components; nor do I think there’s any scenario that requires destroying a nacos group. Moreover, the logic should be that one Nacos server corresponds to one registry instance.
I don’t mean I’m correct. But I think we must find out why Nacos has this special group-related logic, and what scenarios would trigger the destroy logic.
--
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] Reuse Nacos NamingService connections across registry groups [dubbo]
Posted by "somiljain2006 (via GitHub)" <gi...@apache.org>.
somiljain2006 commented on PR #15891:
URL: https://github.com/apache/dubbo/pull/15891#issuecomment-3682029728
No, that would cause different namespaces to collapse into the same cache key. Also, relying solely on RegistryFactory caching is unsafe when multiple services share a Registry instance; one service calling destroy() would physically kill the connection for everyone else.
--
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] Reuse Nacos NamingService connections across registry groups [dubbo]
Posted by "RainYuY (via GitHub)" <gi...@apache.org>.
RainYuY commented on PR #15891:
URL: https://github.com/apache/dubbo/pull/15891#issuecomment-3682102250
> No, that would cause different namespaces to collapse into the same cache key. Also, relying solely on RegistryFactory caching is unsafe when multiple services share a Registry instance; one service calling destroy() would physically kill the connection for everyone else. We need the reference counting to manage that lifecycle.
Regarding the first point, I don’t know what problems the same cache key will cause. In fact, I think the same cache key should yield the correct result—maybe there’s some reason behind the special logic. However, I don’t think the second scenario exists. Because as for the registry, I believe we only destroy it when destroying all components; nor do I think there’s any scenario that requires destroying a nacos group. Moreover, the logic should be that one Nacos server corresponds to one registry instance.
--
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] Reuse Nacos NamingService connections across registry groups [dubbo]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #15891:
URL: https://github.com/apache/dubbo/pull/15891#discussion_r2638978810
##########
dubbo-registry/dubbo-registry-nacos/src/main/java/org/apache/dubbo/registry/nacos/util/NacosNamingServiceUtils.java:
##########
@@ -104,22 +125,103 @@ public static String getGroup(URL connectionURL) {
return connectionURL.getParameter(NACOS_GROUP_KEY, group);
}
+ // This ensures that different registry groups sharing the same Nacos server and namespace reuse a single
+ // physical connection.
+ private static String createNamingServiceCacheKey(URL connectionURL) {
+ URL normalized = normalizeConnectionURL(connectionURL);
+ return normalized.toFullString();
+ }
+
/**
- * Create an instance of {@link NamingService} from specified {@link URL connection url}
+ * Create or obtain a shared {@link NacosNamingServiceWrapper} for the given connection URL.
*
- * @param connectionURL {@link URL connection url}
- * @return {@link NamingService}
+ * @param connectionURL the registry connection URL
+ * @return a shared {@link NacosNamingServiceWrapper}
* @since 2.7.5
*/
public static NacosNamingServiceWrapper createNamingService(URL connectionURL) {
+ String key = createNamingServiceCacheKey(connectionURL);
+
+ // Create or retrieve the shared service holder.
+ NacosNamingServiceHolder holder = SERVICE_CACHE.compute(key, (k, v) -> {
+ if (v == null) {
+ logger.info("Creating shared NacosNamingService for key: {}", key);
+ NacosNamingServiceWrapper newWrapper = createWrapperInternal(connectionURL);
+ v = new NacosNamingServiceHolder(newWrapper);
+ }
+ v.refCount.incrementAndGet();
+ return v;
+ });
+
+ return holder.wrapper;
+ }
+
+ // Release a previously acquired naming service reference.
+ public static void releaseNamingService(URL connectionURL) {
+ String key = createNamingServiceCacheKey(connectionURL);
+
+ // Decrement the reference count and close the connection if it reaches zero.
+ SERVICE_CACHE.compute(key, (k, v) -> {
+ if (v == null) {
+ return null;
+ }
+
+ if (v.refCount.decrementAndGet() <= 0) {
Review Comment:
The condition `v.refCount.decrementAndGet() <= 0` has a race condition risk. If the count becomes negative due to unbalanced release calls (more releases than creates), the wrapper will be shut down and removed from the cache, but subsequent releases will find `v == null` and silently return. This could mask bugs where releaseNamingService is called more times than createNamingService for the same URL.
Consider changing the condition to check `== 0` instead of `<= 0`, and add explicit logging or an assertion when the count goes negative to help detect misuse.
```suggestion
int newRefCount = v.refCount.decrementAndGet();
if (newRefCount < 0) {
logger.warn(REGISTRY_NACOS_EXCEPTION, "", "",
"NacosNamingService reference count for key " + key + " dropped below zero: " + newRefCount);
}
if (newRefCount == 0) {
```
##########
dubbo-registry/dubbo-registry-nacos/src/main/java/org/apache/dubbo/registry/nacos/util/NacosNamingServiceUtils.java:
##########
@@ -104,22 +125,103 @@ public static String getGroup(URL connectionURL) {
return connectionURL.getParameter(NACOS_GROUP_KEY, group);
}
+ // This ensures that different registry groups sharing the same Nacos server and namespace reuse a single
+ // physical connection.
+ private static String createNamingServiceCacheKey(URL connectionURL) {
+ URL normalized = normalizeConnectionURL(connectionURL);
+ return normalized.toFullString();
+ }
+
/**
- * Create an instance of {@link NamingService} from specified {@link URL connection url}
+ * Create or obtain a shared {@link NacosNamingServiceWrapper} for the given connection URL.
*
- * @param connectionURL {@link URL connection url}
- * @return {@link NamingService}
+ * @param connectionURL the registry connection URL
+ * @return a shared {@link NacosNamingServiceWrapper}
* @since 2.7.5
*/
public static NacosNamingServiceWrapper createNamingService(URL connectionURL) {
+ String key = createNamingServiceCacheKey(connectionURL);
+
+ // Create or retrieve the shared service holder.
+ NacosNamingServiceHolder holder = SERVICE_CACHE.compute(key, (k, v) -> {
+ if (v == null) {
+ logger.info("Creating shared NacosNamingService for key: {}", key);
+ NacosNamingServiceWrapper newWrapper = createWrapperInternal(connectionURL);
+ v = new NacosNamingServiceHolder(newWrapper);
+ }
+ v.refCount.incrementAndGet();
+ return v;
+ });
+
+ return holder.wrapper;
+ }
+
+ // Release a previously acquired naming service reference.
Review Comment:
The new public method `releaseNamingService` lacks proper JavaDoc documentation. While the internal comment on line 159 explains its purpose, users of this API need to understand:
1. When and why to call this method
2. That it must be called exactly once for each corresponding createNamingService() call
3. What happens if it's called without a prior createNamingService() call
4. Thread-safety guarantees
Consider adding comprehensive JavaDoc similar to the style used for createNamingService().
```suggestion
/**
* Release a previously acquired shared {@link NacosNamingServiceWrapper} for the given connection URL.
* <p>
* This method must be called exactly once for every successful invocation of
* {@link #createNamingService(URL)} using an equivalent {@link URL} (i.e., one that results in the same
* internal cache key). When the reference count for the corresponding cached entry drops to zero, the
* underlying {@link NamingService} is shut down and removed from the cache.
* </p>
* <p>
* If this method is called with a {@code connectionURL} for which no {@link #createNamingService(URL)}
* call has been made before (or if the associated entry has already been fully released), the call is a
* no-op and no exception is thrown.
* </p>
* <p>
* This method is thread-safe. Reference counting and cache updates are performed atomically using a
* {@link ConcurrentMap} and {@link java.util.concurrent.atomic.AtomicInteger}, so it is safe to call
* {@code createNamingService} and {@code releaseNamingService} concurrently from multiple threads.
* </p>
*
* @param connectionURL the registry connection URL associated with the naming service to release
* @since 2.7.5
*/
```
##########
dubbo-registry/dubbo-registry-nacos/src/main/java/org/apache/dubbo/registry/nacos/util/NacosNamingServiceUtils.java:
##########
@@ -104,22 +125,103 @@ public static String getGroup(URL connectionURL) {
return connectionURL.getParameter(NACOS_GROUP_KEY, group);
}
+ // This ensures that different registry groups sharing the same Nacos server and namespace reuse a single
+ // physical connection.
+ private static String createNamingServiceCacheKey(URL connectionURL) {
+ URL normalized = normalizeConnectionURL(connectionURL);
+ return normalized.toFullString();
+ }
+
/**
- * Create an instance of {@link NamingService} from specified {@link URL connection url}
+ * Create or obtain a shared {@link NacosNamingServiceWrapper} for the given connection URL.
*
- * @param connectionURL {@link URL connection url}
- * @return {@link NamingService}
+ * @param connectionURL the registry connection URL
+ * @return a shared {@link NacosNamingServiceWrapper}
* @since 2.7.5
*/
public static NacosNamingServiceWrapper createNamingService(URL connectionURL) {
+ String key = createNamingServiceCacheKey(connectionURL);
+
+ // Create or retrieve the shared service holder.
+ NacosNamingServiceHolder holder = SERVICE_CACHE.compute(key, (k, v) -> {
+ if (v == null) {
+ logger.info("Creating shared NacosNamingService for key: {}", key);
+ NacosNamingServiceWrapper newWrapper = createWrapperInternal(connectionURL);
+ v = new NacosNamingServiceHolder(newWrapper);
+ }
+ v.refCount.incrementAndGet();
+ return v;
+ });
Review Comment:
There's a subtle race condition in the reference counting logic. If two threads call createNamingService() concurrently with the same key, both could observe `v == null`, create separate NacosNamingServiceHolder instances, and one would overwrite the other. While ConcurrentHashMap.compute() is atomic, the logic inside creates a new holder on line 150 and then increments its refCount on line 152. If the holder gets replaced between creation and incrementing, the reference count will be incorrect.
The current implementation is actually safe because compute() guarantees atomicity of the entire lambda execution, but for clarity and robustness, consider initializing the refCount to 1 in the constructor instead of 0, and only increment it for existing holders. This makes the intent clearer and removes the possibility of future bugs if the code is refactored.
##########
dubbo-registry/dubbo-registry-nacos/src/main/java/org/apache/dubbo/registry/nacos/util/NacosNamingServiceUtils.java:
##########
@@ -104,22 +125,103 @@ public static String getGroup(URL connectionURL) {
return connectionURL.getParameter(NACOS_GROUP_KEY, group);
}
+ // This ensures that different registry groups sharing the same Nacos server and namespace reuse a single
+ // physical connection.
+ private static String createNamingServiceCacheKey(URL connectionURL) {
+ URL normalized = normalizeConnectionURL(connectionURL);
+ return normalized.toFullString();
+ }
+
/**
- * Create an instance of {@link NamingService} from specified {@link URL connection url}
+ * Create or obtain a shared {@link NacosNamingServiceWrapper} for the given connection URL.
*
- * @param connectionURL {@link URL connection url}
- * @return {@link NamingService}
+ * @param connectionURL the registry connection URL
+ * @return a shared {@link NacosNamingServiceWrapper}
* @since 2.7.5
*/
public static NacosNamingServiceWrapper createNamingService(URL connectionURL) {
+ String key = createNamingServiceCacheKey(connectionURL);
+
+ // Create or retrieve the shared service holder.
+ NacosNamingServiceHolder holder = SERVICE_CACHE.compute(key, (k, v) -> {
+ if (v == null) {
+ logger.info("Creating shared NacosNamingService for key: {}", key);
+ NacosNamingServiceWrapper newWrapper = createWrapperInternal(connectionURL);
+ v = new NacosNamingServiceHolder(newWrapper);
+ }
+ v.refCount.incrementAndGet();
+ return v;
+ });
+
+ return holder.wrapper;
+ }
+
+ // Release a previously acquired naming service reference.
+ public static void releaseNamingService(URL connectionURL) {
+ String key = createNamingServiceCacheKey(connectionURL);
+
+ // Decrement the reference count and close the connection if it reaches zero.
+ SERVICE_CACHE.compute(key, (k, v) -> {
+ if (v == null) {
+ return null;
+ }
+
+ if (v.refCount.decrementAndGet() <= 0) {
+ try {
+ logger.info("Destroying shared NacosNamingService for key: {}", key);
+ v.wrapper.shutdown();
+ } catch (Exception e) {
+ logger.warn(
+ REGISTRY_NACOS_EXCEPTION, "", "", "Failed to destroy naming service for key: " + key, e);
+ }
+ return null;
+ }
+ return v;
+ });
+ }
+
+ /**
+ * Create a new {@link NacosNamingServiceWrapper} using the normalized
+ * connection URL and configured retry options.
+ */
+ private static NacosNamingServiceWrapper createWrapperInternal(URL connectionURL) {
+ URL normalized = normalizeConnectionURL(connectionURL);
+
boolean check = connectionURL.getParameter(NACOS_CHECK_KEY, true);
int retryTimes = connectionURL.getPositiveParameter(NACOS_RETRY_KEY, 10);
int sleepMsBetweenRetries = connectionURL.getPositiveParameter(NACOS_RETRY_WAIT_KEY, 10);
+
if (check && !UrlUtils.isCheck(connectionURL)) {
check = false;
}
+
NacosConnectionManager nacosConnectionManager =
- new NacosConnectionManager(connectionURL, check, retryTimes, sleepMsBetweenRetries);
+ new NacosConnectionManager(normalized, check, retryTimes, sleepMsBetweenRetries);
return new NacosNamingServiceWrapper(nacosConnectionManager, retryTimes, sleepMsBetweenRetries);
}
+
+ /**
+ * Normalizes the URL by removing group parameters and sorting the server address list.
+ * This method removes grouping parameters and standardizes the server address list
Review Comment:
The javadoc comment is incomplete - it ends abruptly without explaining what the method does with the standardized server address list. Complete the documentation to explain that addresses are trimmed, deduplicated (via sorted stream), and sorted alphabetically to ensure consistent cache keys regardless of the order addresses are specified in the URL.
```suggestion
* This method removes grouping parameters and standardizes the server address list by trimming
* whitespace, discarding empty entries, deduplicating and sorting addresses alphabetically so
* that the resulting URL produces consistent cache keys regardless of the order in which
* server addresses are specified.
```
##########
dubbo-registry/dubbo-registry-nacos/src/main/java/org/apache/dubbo/registry/nacos/util/NacosNamingServiceUtils.java:
##########
@@ -104,22 +125,103 @@ public static String getGroup(URL connectionURL) {
return connectionURL.getParameter(NACOS_GROUP_KEY, group);
}
+ // This ensures that different registry groups sharing the same Nacos server and namespace reuse a single
+ // physical connection.
+ private static String createNamingServiceCacheKey(URL connectionURL) {
+ URL normalized = normalizeConnectionURL(connectionURL);
+ return normalized.toFullString();
+ }
+
/**
- * Create an instance of {@link NamingService} from specified {@link URL connection url}
+ * Create or obtain a shared {@link NacosNamingServiceWrapper} for the given connection URL.
*
- * @param connectionURL {@link URL connection url}
- * @return {@link NamingService}
+ * @param connectionURL the registry connection URL
+ * @return a shared {@link NacosNamingServiceWrapper}
* @since 2.7.5
*/
public static NacosNamingServiceWrapper createNamingService(URL connectionURL) {
+ String key = createNamingServiceCacheKey(connectionURL);
+
+ // Create or retrieve the shared service holder.
+ NacosNamingServiceHolder holder = SERVICE_CACHE.compute(key, (k, v) -> {
+ if (v == null) {
+ logger.info("Creating shared NacosNamingService for key: {}", key);
+ NacosNamingServiceWrapper newWrapper = createWrapperInternal(connectionURL);
+ v = new NacosNamingServiceHolder(newWrapper);
+ }
+ v.refCount.incrementAndGet();
+ return v;
+ });
+
+ return holder.wrapper;
+ }
+
+ // Release a previously acquired naming service reference.
+ public static void releaseNamingService(URL connectionURL) {
+ String key = createNamingServiceCacheKey(connectionURL);
+
+ // Decrement the reference count and close the connection if it reaches zero.
+ SERVICE_CACHE.compute(key, (k, v) -> {
+ if (v == null) {
+ return null;
+ }
+
+ if (v.refCount.decrementAndGet() <= 0) {
+ try {
+ logger.info("Destroying shared NacosNamingService for key: {}", key);
+ v.wrapper.shutdown();
+ } catch (Exception e) {
+ logger.warn(
+ REGISTRY_NACOS_EXCEPTION, "", "", "Failed to destroy naming service for key: " + key, e);
+ }
+ return null;
+ }
+ return v;
+ });
+ }
+
+ /**
+ * Create a new {@link NacosNamingServiceWrapper} using the normalized
+ * connection URL and configured retry options.
+ */
+ private static NacosNamingServiceWrapper createWrapperInternal(URL connectionURL) {
+ URL normalized = normalizeConnectionURL(connectionURL);
+
boolean check = connectionURL.getParameter(NACOS_CHECK_KEY, true);
int retryTimes = connectionURL.getPositiveParameter(NACOS_RETRY_KEY, 10);
int sleepMsBetweenRetries = connectionURL.getPositiveParameter(NACOS_RETRY_WAIT_KEY, 10);
+
if (check && !UrlUtils.isCheck(connectionURL)) {
check = false;
}
+
NacosConnectionManager nacosConnectionManager =
- new NacosConnectionManager(connectionURL, check, retryTimes, sleepMsBetweenRetries);
+ new NacosConnectionManager(normalized, check, retryTimes, sleepMsBetweenRetries);
return new NacosNamingServiceWrapper(nacosConnectionManager, retryTimes, sleepMsBetweenRetries);
Review Comment:
The normalization logic recreates the URL using `URL.valueOf(connectionURL.toServiceStringWithoutResolving())`, which may not preserve all parameters from the original URL. Parameters like `nacos.retry`, `nacos.retry-wait`, and `nacos.check` are needed in createWrapperInternal() (lines 190-192) but are retrieved from the original `connectionURL` parameter, not from `normalized`.
While this works correctly because createWrapperInternal() receives the original connectionURL as a parameter, it creates a subtle inconsistency: the normalized URL is used for creating the NacosConnectionManager, but retry parameters come from the original URL. This could be confusing for future maintainers. Consider adding a comment explaining why some parameters are taken from the original URL rather than the normalized one.
##########
dubbo-registry/dubbo-registry-nacos/src/test/java/org/apache/dubbo/registry/nacos/util/NacosNamingServiceUtilsTest.java:
##########
@@ -172,4 +175,9 @@ public String getServerStatus() {
}
}
}
+
+ @AfterEach
+ void tearDown() {
+ NacosNamingServiceUtils.clearCacheForTest();
+ }
}
Review Comment:
The test suite is missing coverage for the core connection reuse functionality introduced in this PR. There are no tests that verify:
1. Multiple createNamingService() calls with the same connection parameters return the same wrapper instance
2. The reference counting mechanism works correctly (incrementing on create, decrementing on release)
3. The connection is only shut down when the last reference is released
4. Connections are properly reused across different registry groups
Consider adding a test that creates multiple NamingService instances with different group parameters but the same server address, verifies they share the same underlying connection, then releases them and confirms the connection is only closed after all references are released.
##########
dubbo-registry/dubbo-registry-nacos/src/main/java/org/apache/dubbo/registry/nacos/util/NacosNamingServiceUtils.java:
##########
@@ -104,22 +125,103 @@ public static String getGroup(URL connectionURL) {
return connectionURL.getParameter(NACOS_GROUP_KEY, group);
}
+ // This ensures that different registry groups sharing the same Nacos server and namespace reuse a single
+ // physical connection.
+ private static String createNamingServiceCacheKey(URL connectionURL) {
+ URL normalized = normalizeConnectionURL(connectionURL);
+ return normalized.toFullString();
+ }
+
/**
- * Create an instance of {@link NamingService} from specified {@link URL connection url}
+ * Create or obtain a shared {@link NacosNamingServiceWrapper} for the given connection URL.
*
- * @param connectionURL {@link URL connection url}
- * @return {@link NamingService}
+ * @param connectionURL the registry connection URL
+ * @return a shared {@link NacosNamingServiceWrapper}
* @since 2.7.5
*/
public static NacosNamingServiceWrapper createNamingService(URL connectionURL) {
+ String key = createNamingServiceCacheKey(connectionURL);
+
+ // Create or retrieve the shared service holder.
+ NacosNamingServiceHolder holder = SERVICE_CACHE.compute(key, (k, v) -> {
+ if (v == null) {
+ logger.info("Creating shared NacosNamingService for key: {}", key);
+ NacosNamingServiceWrapper newWrapper = createWrapperInternal(connectionURL);
+ v = new NacosNamingServiceHolder(newWrapper);
+ }
+ v.refCount.incrementAndGet();
+ return v;
+ });
+
+ return holder.wrapper;
+ }
+
+ // Release a previously acquired naming service reference.
+ public static void releaseNamingService(URL connectionURL) {
+ String key = createNamingServiceCacheKey(connectionURL);
+
+ // Decrement the reference count and close the connection if it reaches zero.
+ SERVICE_CACHE.compute(key, (k, v) -> {
+ if (v == null) {
+ return null;
+ }
+
+ if (v.refCount.decrementAndGet() <= 0) {
+ try {
+ logger.info("Destroying shared NacosNamingService for key: {}", key);
+ v.wrapper.shutdown();
+ } catch (Exception e) {
+ logger.warn(
+ REGISTRY_NACOS_EXCEPTION, "", "", "Failed to destroy naming service for key: " + key, e);
+ }
+ return null;
+ }
+ return v;
+ });
+ }
+
+ /**
+ * Create a new {@link NacosNamingServiceWrapper} using the normalized
+ * connection URL and configured retry options.
+ */
+ private static NacosNamingServiceWrapper createWrapperInternal(URL connectionURL) {
+ URL normalized = normalizeConnectionURL(connectionURL);
+
boolean check = connectionURL.getParameter(NACOS_CHECK_KEY, true);
int retryTimes = connectionURL.getPositiveParameter(NACOS_RETRY_KEY, 10);
int sleepMsBetweenRetries = connectionURL.getPositiveParameter(NACOS_RETRY_WAIT_KEY, 10);
+
if (check && !UrlUtils.isCheck(connectionURL)) {
check = false;
}
+
NacosConnectionManager nacosConnectionManager =
- new NacosConnectionManager(connectionURL, check, retryTimes, sleepMsBetweenRetries);
+ new NacosConnectionManager(normalized, check, retryTimes, sleepMsBetweenRetries);
return new NacosNamingServiceWrapper(nacosConnectionManager, retryTimes, sleepMsBetweenRetries);
}
+
+ /**
+ * Normalizes the URL by removing group parameters and sorting the server address list.
+ * This method removes grouping parameters and standardizes the server address list
+ */
+ private static URL normalizeConnectionURL(URL connectionURL) {
+ URL normalized = URL.valueOf(connectionURL.toServiceStringWithoutResolving())
+ .removeParameter(GROUP_KEY)
+ .removeParameter(NACOS_GROUP_KEY);
+
+ String serverAddr = normalized.getParameter("serverAddr", normalized.getAddress());
+ if (serverAddr != null) {
+ String canonical = Arrays.stream(serverAddr.split(","))
+ .map(String::trim)
+ .filter(s -> !s.isEmpty())
+ .sorted()
+ .collect(Collectors.joining(","));
+ normalized = normalized.addParameter("serverAddr", canonical);
+ }
Review Comment:
The fallback `normalized.getAddress()` on line 212 may not provide a meaningful value when `serverAddr` parameter is present in the URL. In Nacos configurations, the server address is typically specified via the `serverAddr` parameter (comma-separated list), not via the URL host/port. If `serverAddr` is null or empty, falling back to a single host from the URL may not accurately represent the connection identity.
Consider whether the normalization should fail or log a warning when neither `serverAddr` parameter nor a valid address is present, as this could indicate a misconfiguration.
```suggestion
// Prefer explicitly configured serverAddr; only fall back to URL address if non-empty.
String serverAddr = normalized.getParameter("serverAddr");
if (serverAddr == null || serverAddr.trim().isEmpty()) {
String addressFromUrl = normalized.getAddress();
if (addressFromUrl != null && !addressFromUrl.trim().isEmpty()) {
serverAddr = addressFromUrl;
} else {
// Neither serverAddr nor a valid URL address is present; this likely indicates misconfiguration.
logger.warn(REGISTRY_NACOS_EXCEPTION, "", "",
"No valid Nacos server address found in URL parameters or host/port: " + connectionURL);
return normalized;
}
}
String canonical = Arrays.stream(serverAddr.split(","))
.map(String::trim)
.filter(s -> !s.isEmpty())
.sorted()
.collect(Collectors.joining(","));
normalized = normalized.addParameter("serverAddr", canonical);
```
--
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] Reuse Nacos NamingService connections across registry groups [dubbo]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #15891:
URL: https://github.com/apache/dubbo/pull/15891#issuecomment-3678725922
## [Codecov](https://app.codecov.io/gh/apache/dubbo/pull/15891?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `74.46809%` with `12 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 32.16%. Comparing base ([`47a141c`](https://app.codecov.io/gh/apache/dubbo/commit/47a141c12a0d4f66d988e1200dd410fec42e3eac?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`6363419`](https://app.codecov.io/gh/apache/dubbo/commit/6363419d051f51f0a84cc3e1128f743e139be93e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: **Current head 6363419 differs from pull request most recent head f02a5dc**
Please [upload](https://docs.codecov.com/docs/codecov-uploader) reports for the commit f02a5dc to get more accurate results.
| [Files with missing lines](https://app.codecov.io/gh/apache/dubbo/pull/15891?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...o/registry/nacos/util/NacosNamingServiceUtils.java](https://app.codecov.io/gh/apache/dubbo/pull/15891?src=pr&el=tree&filepath=dubbo-registry%2Fdubbo-registry-nacos%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fdubbo%2Fregistry%2Fnacos%2Futil%2FNacosNamingServiceUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZHViYm8tcmVnaXN0cnkvZHViYm8tcmVnaXN0cnktbmFjb3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JlZ2lzdHJ5L25hY29zL3V0aWwvTmFjb3NOYW1pbmdTZXJ2aWNlVXRpbHMuamF2YQ==) | 77.27% | [6 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/dubbo/pull/15891?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...org/apache/dubbo/registry/nacos/NacosRegistry.java](https://app.codecov.io/gh/apache/dubbo/pull/15891?src=pr&el=tree&filepath=dubbo-registry%2Fdubbo-registry-nacos%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fdubbo%2Fregistry%2Fnacos%2FNacosRegistry.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZHViYm8tcmVnaXN0cnkvZHViYm8tcmVnaXN0cnktbmFjb3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2R1YmJvL3JlZ2lzdHJ5L25hY29zL05hY29zUmVnaXN0cnkuamF2YQ==) | 33.33% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/dubbo/pull/15891?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (47a141c) and HEAD (6363419). Click for more details.
>
> <details><summary>HEAD has 7 uploads less than BASE</summary>
>
>| Flag | BASE (47a141c) | HEAD (6363419) |
>|------|------|------|
>|integration-tests-java21|1|0|
>|integration-tests-java8|1|0|
>|unit-tests-java11|1|0|
>|unit-tests-java8|1|0|
>|unit-tests-java25|1|0|
>|unit-tests-java21|1|0|
>|unit-tests-java17|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 3.3 #15891 +/- ##
=============================================
- Coverage 60.75% 32.16% -28.59%
+ Complexity 11703 10143 -1560
=============================================
Files 1938 1937 -1
Lines 88694 88716 +22
Branches 13387 13305 -82
=============================================
- Hits 53886 28537 -25349
- Misses 29275 56000 +26725
+ Partials 5533 4179 -1354
```
| [Flag](https://app.codecov.io/gh/apache/dubbo/pull/15891/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration-tests-java21](https://app.codecov.io/gh/apache/dubbo/pull/15891/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration-tests-java8](https://app.codecov.io/gh/apache/dubbo/pull/15891/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [samples-tests-java21](https://app.codecov.io/gh/apache/dubbo/pull/15891/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `32.09% <74.46%> (-0.01%)` | :arrow_down: |
| [samples-tests-java8](https://app.codecov.io/gh/apache/dubbo/pull/15891/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `29.66% <74.46%> (+0.04%)` | :arrow_up: |
| [unit-tests-java11](https://app.codecov.io/gh/apache/dubbo/pull/15891/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit-tests-java17](https://app.codecov.io/gh/apache/dubbo/pull/15891/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit-tests-java21](https://app.codecov.io/gh/apache/dubbo/pull/15891/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit-tests-java25](https://app.codecov.io/gh/apache/dubbo/pull/15891/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit-tests-java8](https://app.codecov.io/gh/apache/dubbo/pull/15891/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/dubbo/pull/15891?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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] Reuse Nacos NamingService connections across registry groups [dubbo]
Posted by "somiljain2006 (via GitHub)" <gi...@apache.org>.
somiljain2006 commented on PR #15891:
URL: https://github.com/apache/dubbo/pull/15891#issuecomment-3682411713
That makes sense. I agree we should understand the intent behind Nacos’s group-related logic and the lifecycle assumptions more clearly.
From what I’ve seen, Nacos treats groups as a naming-level concern, while the NamingService itself is bound to a specific server + namespace. That’s why the Nacos API exposes a group on registration/subscription methods rather than requiring a separate NamingService per group.
On the destroy side, the primary scenario guards against is the shutdown race condition. When multiple registry wrappers share a single NamingService, they are destroyed in sequence during application shutdown. Without reference counting, the first registry to close would call shutdown() on the shared connection, instantly killing it.
--
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