SavitarC commented on PR #16248:
URL: https://github.com/apache/dubbo/pull/16248#issuecomment-4377016902
Thanks for working on this. I noticed a couple of edge cases around how
registration failures are reported and recorded.
The first case is mainly related to standard `FailbackRegistry`
implementations. In `FailbackRegistry.register()`, a failed `doRegister()` does
not always propagate to the caller. When `check=false`, the failure is
converted into a retry task and `register()` returns normally:
```java
try {
doRegister(url);
} catch (Exception e) {
boolean check = getUrl().getParameter(Constants.CHECK_KEY, true)
&& url.getParameter(Constants.CHECK_KEY, true)
&& (url.getPort() != 0);
if (check || skipFailback) {
throw new IllegalStateException(...);
} else {
logger.error(... "waiting for retry" ...);
}
addFailedRegistered(url);
}
```
`addFailedRegistered(url)` then schedules a retry:
```java
FailedRegisteredTask newTask = new FailedRegisteredTask(url, this);
oldOne = failedRegistered.putIfAbsent(url, newTask);
if (oldOne == null) {
retryTimer.newTimeout(newTask, retryPeriod, TimeUnit.MILLISECONDS);
}
```
So for failback registries such as `ZookeeperRegistry`, `NacosRegistry`, or
`ServiceDiscoveryRegistry`, `register()` may return normally even though the
immediate registration attempt failed and is only waiting for retry. In that
case, reporting `SUCCESS` based only on the absence of an exception would be
misleading.
The second case is about the caller-side recorded state.
`registerWithModeTag()` may return `false` when registration fails with
`check=false`, but `export()` appears to ignore that result and record the
stated URL/exporter as registered based on the configured `register` flag:
```java
boolean register = providerUrl.getParameter(REGISTER_KEY, true)
&& registryUrl.getParameter(REGISTER_KEY, true);
if (register) {
registerWithModeTag(registry, registryUrl, registeredProviderUrl);
}
registerStatedUrl(registryUrl, registeredProviderUrl, register);
exporter.setRegistered(register);
```
Similarly, `ExporterChangeableWrapper.register()` should probably avoid
keeping its atomic registered flag set when registration returns `false` or
throws. Otherwise, later manual register/re-register logic may incorrectly
believe registration has already succeeded.
Could we propagate the actual registration outcome into these recorded
states and add regression coverage for these paths? In particular, it would be
helpful to cover a realistic `FailbackRegistry` path where `doRegister()` fails
under `check=false` but `Registry.register()` returns normally, since the
current failure coverage appears to focus on the direct-throwing
`Registry.register()` path.
--
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]
