Feat support check for config-center and metadata-center #15639
Conversation
…and service-discovery
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 3.3 #15639 +/- ##
============================================
+ Coverage 61.02% 61.05% +0.02%
- Complexity 11685 11715 +30
============================================
Files 1923 1924 +1
Lines 87081 87223 +142
Branches 13115 13136 +21
============================================
+ Hits 53141 53252 +111
- Misses 28488 28491 +3
- Partials 5452 5480 +28
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…nter_and_metadata' into feat_support_check_for_config_center_and_metadata
…sAvailable method.
There was a problem hiding this comment.
Pull Request Overview
This PR implements support for the check=false option across config-center, metadata-center, and service discovery components in Dubbo, allowing these services to continue operating when their registry connections are unavailable. The changes also add failure retry mechanisms for metadata reporting and service registration.
- Adds
isAvailable()methods to check connection status for DynamicConfiguration and MetadataReport interfaces - Implements retry logic for service name mapping and metadata reporting when connections fail
- Propagates registry
checkparameter to config-center and metadata-center configurations
Reviewed Changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| dubbo-common/src/main/java/org/apache/dubbo/common/utils/UrlUtils.java | Adds utility method to check if connection checking is enabled |
| dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/deploy/DefaultApplicationDeployer.java | Propagates registry check option to config-center and metadata-center |
| dubbo-configcenter/dubbo-configcenter-/src/main/java/org/apache/dubbo/configcenter/support//NacosDynamicConfiguration.java | Implements connection availability checking for Nacos config center |
| dubbo-metadata/dubbo-metadata-api/src/main/java/org/apache/dubbo/metadata/AbstractServiceNameMapping.java | Adds retry mechanism for service name mapping registration |
| dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/ServiceDiscoveryRegistry.java | Adds failure retry support for consumer subscriptions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/bootstrap/DubboBootstrap.java
Outdated
Show resolved
Hide resolved
...-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/bootstrap/DubboBootstrap.java
Outdated
Show resolved
Hide resolved
| e); | ||
| } |
There was a problem hiding this comment.
[nitpick] The nested try-catch with retry logic inside the do-while loop creates complex control flow. Consider extracting this retry logic into a separate method for better readability and maintainability.
| e); | |
| } | |
| e); | |
| return false; | |
| } | |
| } | |
| /** | |
| * Helper method to perform mapping with retry logic. | |
| */ | |
| private boolean doMappingWithRetry(MetadataReport metadataReport, URL url) throws InterruptedException { | |
| boolean succeeded = false; | |
| int currentRetryTimes = 1; | |
| do { | |
| succeeded = super.doMapping(metadataReport, url); | |
| if (succeeded) { | |
| logger.info( | |
| "[METADATA_REGISTER] [SERVICE_NAME_MAPPING] Successfully registered interface application mapping for service " | |
| + url.getServiceKey()); | |
| break; | |
| } else { | |
| int waitTime = ThreadLocalRandom.current().nextInt(casRetryWaitTime); | |
| logger.info("Failed to publish service name mapping to metadata center by cas operation. " | |
| + "Times: " + currentRetryTimes + ". " | |
| + "Next retry delay: " + waitTime + ". " | |
| + "Service Interface: " + url.getServiceInterface() + ". "); | |
| Thread.sleep(waitTime); | |
| } | |
| } while (currentRetryTimes++ <= casRetryTimes); |
There was a problem hiding this comment.
This is the logic of the implementation class, it overridden method boolean doMapping(MetadataReport metadataReport, URL url).
…ld close retryTimer.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for the check=false configuration option for config-center and metadata-center components in Dubbo. The main purpose is to allow these components to handle connection failures gracefully when check is disabled, implementing retry mechanisms and fail-safe behaviors similar to how registry components already handle the check option.
Key changes include:
- Added
isAvailable()methods to check connection status for DynamicConfiguration and MetadataReport interfaces - Implemented fail-safe behavior when
check=falsefor ZooKeeper and Nacos implementations - Added retry mechanisms for service name mapping and metadata reporting
- Enhanced service discovery to support connection availability checks
Reviewed Changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| dubbo-common/src/main/java/org/apache/dubbo/common/utils/UrlUtils.java | Added utility method to check URL check parameter |
| dubbo-remoting/dubbo-remoting-zookeeper-curator5/src/main/java/org/apache/dubbo/remoting/zookeeper/curator5/Curator5ZookeeperClient.java | Updated to respect check parameter for ZooKeeper connections |
| dubbo-configcenter/dubbo-configcenter-zookeeper/src/main/java/org/apache/dubbo/configcenter/support/zookeeper/ZookeeperDynamicConfiguration.java | Added availability check and removed mandatory connection validation |
| dubbo-metadata/dubbo-metadata-api/src/main/java/org/apache/dubbo/metadata/AbstractServiceNameMapping.java | Added retry mechanism for service name mapping |
| dubbo-config/dubbo-config-api/src/main/java/org/apache/dubbo/config/deploy/DefaultApplicationDeployer.java | Updated to inherit check parameter from registry to config/metadata centers |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...a/dubbo-metadata-api/src/main/java/org/apache/dubbo/metadata/AbstractServiceNameMapping.java
Outdated
Show resolved
Hide resolved
| throw new IllegalStateException(e); | ||
| CONFIG_FAILED_INIT_CONFIG_CENTER, "", "", "The configuration center failed to initialize"); | ||
| if (!configCenter.isCheck()) { | ||
| configCenter.setInitialized(true); |
There was a problem hiding this comment.
initializedwas already set to true atconfigCenter.checkOrUpdateInitialized(true).- you might miss the process when dynamicConfiguration.isAvailable() return false and configCenter.isCheck() returns true.
There was a problem hiding this comment.
- yes, the
configCenter.setInitialized(true)is unnecessary, i'll delete it later. - if check = true, the create process will always throw
IllegalStateException.
There was a problem hiding this comment.
try {
dynamicConfiguration = getDynamicConfiguration(configCenter.toUrl());
if (!dynamicConfiguration.isAvailable()) {
logger.warn(
CONFIG_FAILED_INIT_CONFIG_CENTER, "", "", "The configuration center failed to initialize");
if (!configCenter.isCheck()) {
configCenter.setInitialized(true);
// TODO should it `updateExternalConfigMap` when the connection recovery
return dynamicConfiguration;
}
}
} catch (Exception e) {
throw new IllegalStateException(e);
}
it seemed you don't throw exception when check is true.
There was a problem hiding this comment.
When executing dynamicConfiguration = getDynamicConfiguration(configCenter.toUrl());, SPI DynamicConfigurationFactory will create DynamicConfiguration. Currently the impl nacos zookeeper and apollo will throw IllegalStateException if the check = true and the connection is unavailable.
Do you mean here need throw exception when check=true and connection is not available, to prevent situations where this logic is not implemented in other extensions?
There was a problem hiding this comment.
we don't know customer extensions whether throw exception or not, it might be better to prevent such situations.
There was a problem hiding this comment.
All Right, it was fixed and also the metadata-report client.
zrlw
added
the
type/enhancement
| } | ||
| if (!dynamicConfiguration.isAvailable() && !configCenter.isCheck()) { | ||
| logger.warn( | ||
| "The configuration center initialize successfully. but connection is available now, and the config-center.check is false, it will return now."); |
There was a problem hiding this comment.
but connection is available now,
unavailable ?
There was a problem hiding this comment.
Sorry, it was a mistake here. I have corrected it. Could you please take another look?
This comment was marked as abuse.
This comment was marked as abuse.
|
@zrlw Does this PR contain too many changes that make code review inconvenient? Would it be better for me to split these changes into multiple PRs? |
|
you should fix your error code inspect failure issue first. |
Hey, it was fixed. @zrlw |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 42 out of 42 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| DefaultServiceInstance newServiceInstance = | ||
| new DefaultServiceInstance((DefaultServiceInstance) serviceInstance); | ||
| newServiceInstance | ||
| .getMetadata() | ||
| .put( | ||
| EXPORTED_SERVICES_REVISION_PROPERTY_NAME, | ||
| newServiceInstance.getServiceMetadata().getRevision()); | ||
| doRegister(newServiceInstance); | ||
| this.serviceInstance = newServiceInstance; |
There was a problem hiding this comment.
[nitpick] The metadata update logic is duplicated between the register() and update() methods. Consider extracting this metadata setting logic into a helper method to reduce code duplication.
| .map(InstanceInfo::getInstance) | ||
| .collect(Collectors.toList()); | ||
|
|
||
| accept(() -> namingService.batchRegisterInstance(nacosServiceName, group, instanceListToRegister)); |
There was a problem hiding this comment.
The instance list modification should be done atomically after the successful operation, not during the batch registration process. Moving these operations after the successful batch registration would ensure consistency in case of failures.
| accept(() -> namingService.batchRegisterInstance(nacosServiceName, group, instanceListToRegister)); | |
| accept(() -> namingService.batchRegisterInstance(nacosServiceName, group, instanceListToRegister)); | |
| // Only modify the instance list after successful batch registration |
| for (URL url : urlSet) { | ||
| try { | ||
| if (!retryHandler.apply(metadataReport, url)) { | ||
| throw new Exception("method doMap() return false"); |
There was a problem hiding this comment.
The error message 'method doMap() return false' is unclear and not helpful for debugging. Consider providing a more descriptive message that explains what failed and why.
| throw new Exception("method doMap() return false"); | |
| throw new Exception("Retry handler failed for service url: " + url + ", metadata-center url: " + metadataReport.getUrl()); |
| return !newRevision.equals(metadataInfo.getReportedRevision()) | ||
| || !newRevision.equals(instance.getMetadata(EXPORTED_SERVICES_REVISION_PROPERTY_NAME)); |
There was a problem hiding this comment.
[nitpick] This complex boolean expression should be simplified or broken down into separate boolean variables with descriptive names to improve readability and maintainability.
| return !newRevision.equals(metadataInfo.getReportedRevision()) | |
| || !newRevision.equals(instance.getMetadata(EXPORTED_SERVICES_REVISION_PROPERTY_NAME)); | |
| boolean isReportedRevisionDifferent = !newRevision.equals(metadataInfo.getReportedRevision()); | |
| boolean isExportedServicesRevisionDifferent = !newRevision.equals(instance.getMetadata(EXPORTED_SERVICES_REVISION_PROPERTY_NAME)); | |
| return isReportedRevisionDifferent || isExportedServicesRevisionDifferent; |
| applicationModel.getApplicationConfigManager().getApplication(); | ||
| if (application.isPresent()) { | ||
| enableFileCache = Boolean.TRUE.equals(application.get().getEnableFileCache()) ? true : false; | ||
| enableFileCache = Boolean.TRUE.equals(application.get().getEnableFileCache()); |
There was a problem hiding this comment.
This boolean expression can be simplified. The ternary operator and explicit true/false values are unnecessary since Boolean.TRUE.equals() already returns a boolean.
|
@RainYuY Hi, Could you please take some time to review this PR? |
The next week. |
thanks for the update |
| } | ||
|
|
||
| public static boolean isCheck(URL url) { | ||
| return url.getParameter(CHECK_KEY, true) && url.getPort() != 0; |
There was a problem hiding this comment.
why check url.getPort() != 0?
There was a problem hiding this comment.
why check url.getPort() != 0?
It just kept the original logic, maybe it can be removed.
| .getBean(FrameworkExecutorRepository.class) | ||
| .getSharedScheduledExecutor(); | ||
| mapServiceName(url, serviceNameMapping, scheduledExecutor); | ||
| mapServiceName(url, serviceNameMapping); |
There was a problem hiding this comment.
why remove scheduledExecutor
There was a problem hiding this comment.
It was move to a new class https://github.com/apache/dubbo/blob/eb728417846025108aa61fb7ae777018b9ea96af/dubbo-metadata/dubbo-metadata-api/src/main/java/org/apache/dubbo/metadata/report/MetadataReportRetryTask.java
It will only retry metadata-report client that fail to report.
| new AbortPolicyWithReport(threadName, url)); | ||
|
|
||
| zkClient = zookeeperClientManager.connect(url); | ||
| boolean isConnected = zkClient.isConnected(); |
There was a problem hiding this comment.
Will this deletion possibly affect prepareEnvironment?
There was a problem hiding this comment.
if the check = false and connection is unavailable, the client will add to cache, but will not assign the externalConfiguration and appExternalConfiguration.
Is it necessary to assign the externalConfiguration and appExternalConfiguration when connection recovery?
|
Before I do a detailed review of this PR, I’d like to ask you to reopen PR #15594 |
4 participants
What is the purpose of the change?
resolve #15623 , to support the options
check=falsefor registry-center config-center and metadata-center. There contains the content of PR #15594. The specific changes are as follows:config-center
registry.checkto the config-center or metadata-center config option.related change files:
boolean isAvailable()onDynamicConfiguration, to check the connection status.related change files:
3. Cache the operation of register config item listener when the connection is not available.related change files:- org.apache.dubbo.common.config.configcenter.wrapper.FailbackDynamicConfiguration- org.apache.dubbo.configcenter.support.nacos.NacosDynamicConfigurationFactory- org.apache.dubbo.configcenter.support.zookeeper.ZookeeperDynamicConfigurationFactoryservice-name-mapping
ServiceNameMapping, let it support failure retries.related change files:
metadata
boolean isAvailable()onMetadataReport, to check the connection status.related change files:
check=falserelated change files:
service-discovery
boolean reportedto record whether the metadata reporting is successful or not, Add fieldboolean registeredto record where the service-instance register is successful or not.related change files:
related change files:
consumer
Support the consumer
checkoption, if it isfalse, the subscribe fail will retry byFailbackRegistry.related change files:
Checklist