Aias00 commented on code in PR #7142:
URL: https://github.com/apache/shenyu/pull/7142#discussion_r4060323352
##########
shenyu-register-center/shenyu-register-client/shenyu-register-client-api/src/main/java/org/apache/shenyu/register/client/api/FailbackRegistryRepository.java:
##########
@@ -166,19 +168,28 @@ protected <T> void addFailureMcpDocRegister(final T t) {
if (t instanceof McpToolsRegisterDTO) {
McpToolsRegisterDTO dto = (McpToolsRegisterDTO) t;
MetaDataRegisterDTO metaDataRegisterDTO =
dto.getMetaDataRegisterDTO();
- String address = metaDataRegisterDTO.getRpcType() + "://"
- + metaDataRegisterDTO.getHost() + ":" +
metaDataRegisterDTO.getPort() + metaDataRegisterDTO.getPath();
+ String address = String.join(":", value(dto.getNamespaceId()),
metaDataIdentity(metaDataRegisterDTO));
addToFail(new Holder(dto, address, Constants.MCP_TOOLS_TYPE));
}
}
- private <T> void addToFail(final Holder t) {
- Holder oldObj = concurrentHashMap.get(t.getKey());
+ private static String metaDataIdentity(final MetaDataRegisterDTO dto) {
+ return String.join(":", value(dto.getNamespaceId()),
value(dto.getRpcType()), value(dto.getAppName()),
+ value(dto.getContextPath()), value(dto.getServiceName()),
value(dto.getMethodName()),
+ value(dto.getParameterTypes()), value(dto.getRuleName()),
value(dto.getHost()), value(dto.getPort()), value(dto.getPath()));
+ }
+
+ private static String value(final Object value) {
+ return Objects.toString(value, "");
+ }
+
+ private void addToFail(final Holder t) {
Review Comment:
`put`-first-then-check is strictly better than `get`-then-`put` here, and
replacing the stale payload is the right semantic. But the replacement path
never re-arms the retry, which makes the new behaviour quietly lossy in one
case.
`FailureRegistryTask` is constructed as `super(key,
TimeUnit.SECONDS.toMillis(10), 18)`, i.e. `retryCount = 18`, `retryLimit = (18
< 0) == false`. In `AbstractRetryTask#run`:
```java
if (!retryLimit && tickCount > retryCount) {
logger.warn("Final failed to execute task, key:{}, retried:{}, task
over.", key, tickCount);
return; // gives up - no reschedule, and no
remove(key)
}
```
When it gives up, the `Holder` stays in `concurrentHashMap` forever
(`remove(key)` is only called on success, and only `accept()` - which throws -
precedes it). So the sequence is:
1. registration for identity X fails -> holder stored, task scheduled;
2. task retries 18x over ~180s, all failing -> logs "task over", returns,
holder **still in the map**;
3. any later failure for X hits `oldObj != null` -> returns early -> **no
new task is ever scheduled**.
With this change we now also log `Updated failback registration payload,
...`, which reads as "will be retried" - but nothing will retry it. Before the
change the entry was at least created once; now the sticky-entry window is the
normal path for any long outage.
Suggested fix in this PR, pick one:
- have `addToFail` re-arm a `FailureRegistryTask` when it replaces a payload
whose previous task is no longer live (e.g. keep a `Set<String>` of live keys,
or let `FailureRegistryTask` remove itself from the map when retries are
exhausted and let `addToFail` treat "absent" as "schedule"); or
- add a terminal hook so `AbstractRetryTask#run` can remove the key when it
gives up, making step 3 fall through to the "new failure" branch naturally.
The second is cleaner but touches shared timer code, so the first (re-arm on
replace) is probably the smaller blast radius.
Minor: the new `logger.warn("Updated failback registration payload, ...")`
fires on every duplicate failure, so a bulk re-registration (one entry per
method) produces one WARN per method. Consider `debug` for the replacement case
and keep `warn` for the first-time enqueue below.
--
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]
