Copilot commented on code in PR #15639:
URL: https://github.com/apache/dubbo/pull/15639#discussion_r2340733712
##########
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/AbstractServiceDiscovery.java:
##########
@@ -167,7 +167,15 @@ public synchronized void register() throws
RuntimeException {
if (revisionUpdated) {
try {
reportMetadata(this.metadataInfo);
- doRegister(this.serviceInstance);
+ DefaultServiceInstance newServiceInstance =
+ new DefaultServiceInstance((DefaultServiceInstance)
serviceInstance);
+ newServiceInstance
+ .getMetadata()
+ .put(
+ EXPORTED_SERVICES_REVISION_PROPERTY_NAME,
+
newServiceInstance.getServiceMetadata().getRevision());
+ doRegister(newServiceInstance);
+ this.serviceInstance = newServiceInstance;
Review 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.
##########
dubbo-registry/dubbo-registry-nacos/src/main/java/org/apache/dubbo/registry/nacos/NacosNamingServiceWrapper.java:
##########
@@ -201,21 +201,22 @@ public void updateInstance(String serviceName, String
group, Instance oldInstanc
}
InstanceInfo oldInstanceInfo = optional.get();
- instancesInfo.getInstances().remove(oldInstanceInfo);
- instancesInfo.getInstances().add(new InstanceInfo(newInstance,
oldInstanceInfo.getNamingService()));
-
if (isSupportBatchRegister && instancesInfo.isBatchRegistered()) {
NamingService namingService =
oldInstanceInfo.getNamingService();
List<Instance> instanceListToRegister =
instancesInfo.getInstances().stream()
.map(InstanceInfo::getInstance)
.collect(Collectors.toList());
accept(() ->
namingService.batchRegisterInstance(nacosServiceName, group,
instanceListToRegister));
Review 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.
```suggestion
accept(() ->
namingService.batchRegisterInstance(nacosServiceName, group,
instanceListToRegister));
// Only modify the instance list after successful batch
registration
```
##########
dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/AbstractServiceDiscovery.java:
##########
@@ -377,18 +395,14 @@ protected ServiceInstance
createServiceInstance(MetadataInfo metadataInfo) {
}
protected boolean calOrUpdateInstanceRevision(ServiceInstance instance) {
- String existingInstanceRevision =
getExportedServicesRevision(instance);
MetadataInfo metadataInfo = instance.getServiceMetadata();
String newRevision = metadataInfo.calAndGetRevision();
- if (!newRevision.equals(existingInstanceRevision)) {
-
instance.getMetadata().put(EXPORTED_SERVICES_REVISION_PROPERTY_NAME,
metadataInfo.getRevision());
- return true;
- }
- return false;
+ return !newRevision.equals(metadataInfo.getReportedRevision())
+ ||
!newRevision.equals(instance.getMetadata(EXPORTED_SERVICES_REVISION_PROPERTY_NAME));
Review Comment:
[nitpick] This complex boolean expression should be simplified or broken
down into separate boolean variables with descriptive names to improve
readability and maintainability.
```suggestion
boolean isReportedRevisionDifferent =
!newRevision.equals(metadataInfo.getReportedRevision());
boolean isExportedServicesRevisionDifferent =
!newRevision.equals(instance.getMetadata(EXPORTED_SERVICES_REVISION_PROPERTY_NAME));
return isReportedRevisionDifferent ||
isExportedServicesRevisionDifferent;
```
##########
dubbo-metadata/dubbo-metadata-api/src/main/java/org/apache/dubbo/metadata/report/MetadataReportRetryTask.java:
##########
@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.dubbo.metadata.report;
+
+import org.apache.dubbo.common.URL;
+import org.apache.dubbo.common.logger.ErrorTypeAwareLogger;
+import org.apache.dubbo.common.logger.LoggerFactory;
+import org.apache.dubbo.common.threadpool.manager.FrameworkExecutorRepository;
+import org.apache.dubbo.common.utils.ConcurrentHashSet;
+import org.apache.dubbo.config.ApplicationConfig;
+import org.apache.dubbo.rpc.model.ApplicationModel;
+
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+import java.util.function.BiFunction;
+
+import static
org.apache.dubbo.common.constants.LoggerCodeConstants.CONFIG_SERVER_DISCONNECTED;
+
+public class MetadataReportRetryTask {
+ protected final ErrorTypeAwareLogger logger =
LoggerFactory.getErrorTypeAwareLogger(getClass());
+ private final Object startLock = new Object();
+ private volatile ScheduledFuture<?> future;
+ private final Map<MetadataReport, Set<URL>> taskQueue = new
ConcurrentHashMap<>();
+ private final int mappingRetryInterval;
+ private final ScheduledExecutorService scheduledExecutor;
+ private BiFunction<MetadataReport, URL, Boolean> retryHandler;
+
+ public MetadataReportRetryTask(ApplicationModel applicationModel) {
+ this.mappingRetryInterval = applicationModel
+ .getApplicationConfigManager()
+ .getApplication()
+ .map(ApplicationConfig::getMappingRetryInterval)
+ .orElse(5000);
+ this.scheduledExecutor = applicationModel
+ .getBeanFactory()
+ .getBean(FrameworkExecutorRepository.class)
+ .getSharedScheduledExecutor();
+ }
+
+ public void setRetryHandler(BiFunction<MetadataReport, URL, Boolean>
retryHandler) {
+ this.retryHandler = retryHandler;
+ }
+
+ public void addTask(MetadataReport metadataReport, URL url) {
+ taskQueue
+ .computeIfAbsent(metadataReport, k -> new
ConcurrentHashSet<>())
+ .add(url);
+ }
+
+ /**
+ * start retry task once
+ * @return bool is retry task running
+ */
+ public boolean start() {
+ if (future == null && !taskQueue.isEmpty()) {
+ synchronized (startLock) {
+ if (future == null && !taskQueue.isEmpty()) {
+ future = scheduledExecutor.scheduleWithFixedDelay(
+ this::retry, mappingRetryInterval,
mappingRetryInterval, TimeUnit.MILLISECONDS);
+ }
+ }
+ }
+ return future != null;
+ }
+
+ /**
+ * stop retry task
+ */
+ public void cancel() {
+ if (future != null) {
+ future.cancel(false);
+ future = null;
+ }
+ }
+
+ private void retry() {
+ // stop task if there is no task
+ if (taskQueue.isEmpty()) {
+ cancel();
+ return;
+ }
+ for (Entry<MetadataReport, Set<URL>> entry : taskQueue.entrySet()) {
+ MetadataReport metadataReport = entry.getKey();
+ if (!metadataReport.isAvailable()) {
+ logger.warn(
+ CONFIG_SERVER_DISCONNECTED,
+ "connect is not available",
+ "metadata-center url: " + metadataReport.getUrl(),
+ "[METADATA_REGISTER] [SERVICE_NAME_MAPPING] Retry
Failed.");
+ continue;
+ }
+ Set<URL> urlSet = taskQueue.remove(metadataReport);
+ for (URL url : urlSet) {
+ try {
+ if (!retryHandler.apply(metadataReport, url)) {
+ throw new Exception("method doMap() return false");
Review 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.
```suggestion
throw new Exception("Retry handler failed for
service url: " + url + ", metadata-center url: " + metadataReport.getUrl());
```
##########
dubbo-metadata/dubbo-metadata-api/src/main/java/org/apache/dubbo/metadata/AbstractServiceNameMapping.java:
##########
@@ -53,14 +63,18 @@ public abstract class AbstractServiceNameMapping implements
ServiceNameMapping {
private final ConcurrentHashMap<String, Set<MappingListener>>
mappingListeners = new ConcurrentHashMap<>();
// mapping lock is shared among registries of the same application.
private final ConcurrentMap<String, ReentrantLock> mappingLocks = new
ConcurrentHashMap<>();
+ protected MetadataReportInstance metadataReportInstance;
+ protected static final List<String> IGNORED_SERVICE_INTERFACES =
+ Collections.singletonList(MetadataService.class.getName());
+ protected final MetadataReportRetryTask metadataReportRetryTask;
public AbstractServiceNameMapping(ApplicationModel applicationModel) {
this.applicationModel = applicationModel;
boolean enableFileCache = true;
Optional<ApplicationConfig> application =
applicationModel.getApplicationConfigManager().getApplication();
if (application.isPresent()) {
- enableFileCache =
Boolean.TRUE.equals(application.get().getEnableFileCache()) ? true : false;
+ enableFileCache =
Boolean.TRUE.equals(application.get().getEnableFileCache());
Review 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.
--
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]
