Repository navigation
Conversation
utafrali
left a comment
utafrali
left a comment
There was a problem hiding this comment.
The fix is correct and addresses a real bug where a failure on servers 1..N-1 was silently discarded whenever the last server succeeded. The accumulate-then-throw pattern is clean, and the new parameterized tests meaningfully cover first/last server and multi-server failure cases. Minor gaps: three new test methods break the public void convention used by every existing test in the class, the all-servers-fail heartbeat path has no test, and the sendHeartbeat(InstanceBeatInfoDTO) overload is completely untested for partial failures.
| @@ -85,6 +87,54 @@ public void persistUriShouldRegisterToEveryServer() { | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The three new test methods (partialRegistrationFailureMustPropagate, partialHeartbeatFailureMustPropagate, retainsAllRegistrationFailures) are declared package-private, while every existing test in this class is public void. Pick one style and apply it consistently. JUnit 5 supports both, but mixing them makes the class harder to scan.
// change to match the surrounding class style
public void partialRegistrationFailureMustPropagate(final String failedServer) throws IOException {| registerUtils.verify(() -> RegisterUtils.doHeartBeat(anyString(), eq(server + Constants.URI_PATH), eq(Constants.HEARTBEAT), eq(TOKEN))); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
retainsAllRegistrationFailures covers the all-fail registration path, but there is no equivalent test for the heartbeat path. doHeartbeat now accumulates errors the same way, so a symmetrical test would be:
@Test
public void retainsAllHeartbeatFailures() throws IOException {
HttpClientRegisterRepository multiServerRepository =
new HttpClientRegisterRepository(config(FIRST_SERVER + "," + SECOND_SERVER));
try (MockedStatic<RegisterUtils> registerUtils = mockStatic(RegisterUtils.class);
MockedStatic<RuntimeUtils> runtimeUtils = mockStatic(RuntimeUtils.class)) {
runtimeUtils.when(() -> RuntimeUtils.listenByOther(anyInt())).thenReturn(false);
registerUtils.when(() -> RegisterUtils.doLogin(anyString(), anyString(), anyString())).thenReturn(Optional.of(TOKEN));
registerUtils.when(() -> RegisterUtils.doHeartBeat(anyString(), anyString(), anyString(), anyString())).thenThrow(new IOException("unavailable"));
RuntimeException failure = assertThrows(RuntimeException.class,
() -> multiServerRepository.sendHeartbeat(uriRegisterDTO()));
assertEquals(1, failure.getSuppressed().length);
assertTrue(failure.getCause() instanceof IOException);
}
}| } | ||
| } | ||
|
|
||
| @ParameterizedTest |
There was a problem hiding this comment.
partialHeartbeatFailureMustPropagate only tests sendHeartbeat(URIRegisterDTO). There is a second overload, sendHeartbeat(InstanceBeatInfoDTO), that goes to doHeartbeat via Constants.BEAT_URI_PATH. The failure-propagation logic in doHeartbeat applies to both, but the InstanceBeatInfoDTO path has no coverage at all. A quick partial-failure test mirroring the URI variant (with doHeartBeat stubbed to throw on one server URL) would close that gap.
| } else { | ||
| failure.addSuppressed(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Throwing here when any server failed changes the failure-propagation contract visible to FailbackRegistryRepository. When the failback retrier re-invokes doPersistURI (or related methods), it calls doRegister again for every server in serverList, including any that already succeeded in the previous attempt. For idempotent admin-server endpoints this is harmless, but it is a non-obvious side-effect. A short comment here (or in doRegister's Javadoc) would help future readers understand why duplicate successful registrations can occur on retry:
// Throw after visiting every server so that partial failures are not silently dropped.
// The FailbackRegistryRepository retry will re-attempt all servers, including ones
// that already succeeded; admin-server endpoints are expected to be idempotent.
if (Objects.nonNull(failure)) {
throw failure;
}
Aias00
left a comment
Aias00
left a comment
There was a problem hiding this comment.
Summary
The registration half of this patch is a clear improvement and I am happy with it. Trying every configured admin server, keeping the first cause and attaching the rest as suppressed, then failing if anything failed - that matches what FailbackRegistryRepository.java:77-82 already expects: it catches Exception from doPersistURI and enqueues the DTO for retry. On master a partial failure was silently swallowed while the comment claimed "will retry", so nothing was ever retried. The new doRegister fixes that properly, and retainsAllRegistrationFailures pins it.
Blocking: applying the identical treatment to heartbeats kills the heartbeat loop.
Request changes: the heartbeat exception now permanently stops the scheduler
The heartbeat path has no retry queue and no exception boundary:
ShenyuClientURIExecutorSubscriber.java:74 executor.scheduleAtFixedRate(() -> uris.forEach(this::sendHeartbeat), 30, 10, SECONDS)
ShenyuClientURIExecutorSubscriber.java:126 private void sendHeartbeat(...) { shenyuClientRegisterRepository.sendHeartbeat(uriRegisterDTO); } // no try/catch
HttpClientRegisterRepository.java:138-143 public void sendHeartbeat(URIRegisterDTO dto) { ... doHeartbeat(dto, Constants.URI_PATH); }
HttpClientRegisterRepository.java:251-253 if (Objects.nonNull(failure)) { throw failure; } // NEW: throws when ANY server failed
ScheduledThreadPoolExecutor suppresses every subsequent run of a periodic task once it throws. I reproduced it with a two-admin loop, one admin down, 50 ms period, 1 second window (~20 expected ticks), comparing master's i == serverList.size() rule against this patch's fail-on-any rule:
failFastAnyServer=false -> executed: 21, threw: 0
failFastAnyServer=true -> executed: 1, threw: 1
=> ScheduledThreadPoolExecutor suppressed every subsequent run after the first failure
So with two admins configured, one failover drill (or one network blip) stops this client's heartbeats permanently - until the client process restarts. Admin then ages the URI out and stops routing to it, which is exactly the outage type this change is trying to prevent. Note it also aborts uris.forEach mid-cycle, so other registered URIs lose that beat too.
To be fair to master: the hazard already existed when the last server in the list failed. What this patch does is turn a rare edge case into the common one, because now any single server failure trips it.
Pick whichever you prefer, I do not have a strong opinion:
- keep the accumulation in
doRegister(where the failback wrapper consumes it correctly) and do not throw fromdoHeartbeat- there is no retry queue behind it, so the extra information is only ever lost; or - keep throwing, and add a
try/catchper URI inShenyuClientURIExecutorSubscriber's scheduled body so one bad server cannot cancel the whole loop. This one also preserves the multi-URI isolation thaturis.forEachcurrently lacks.
Please also address the scope question
HeartbeatListener.java:114-131 (bootstrap instance heartbeats) still contains the same int i = 0; ... if (i == serverList.size()) throw loop, and it runs on ScheduledThreadPoolExecutor(1) via scheduleAtFixedRate at :102 - so it carries both the original swallowing bug and, when the last server fails, the same permanent-suppression hazard. Since this PR is precisely about propagating partial failures, please either align it or say in the description that it is deliberately out of scope, so the next person does not assume it was handled.
Minor, non-blocking: only the first failure keeps its cause, later ones become suppressed - fine for triage, but the log line would be more useful with the server identity and the cause, since currently the per-server message omits it for heartbeats.
| } | ||
| } | ||
| } | ||
| if (Objects.nonNull(failure)) { |
There was a problem hiding this comment.
Request changes - see review body. This exception has nowhere to go.
The heartbeat path has no retry queue and no exception boundary between here and the scheduler:
ShenyuClientURIExecutorSubscriber.java:74 executor.scheduleAtFixedRate(() -> uris.forEach(this::sendHeartbeat), 30, 10, SECONDS)
ShenyuClientURIExecutorSubscriber.java:126 private void sendHeartbeat(...) { repository.sendHeartbeat(uri); } // no try/catch
HttpClientRegisterRepository.java:138-143 public void sendHeartbeat(URIRegisterDTO dto) { doHeartbeat(dto, URI_PATH); }
HttpClientRegisterRepository.java:251 throw failure; // NEW: fires when ANY server failed
ScheduledThreadPoolExecutor suppresses all future runs of a periodic task once it throws. Reproduced with two admins, one down, 50 ms period, 1 s window (~20 expected ticks):
master rule (throw only when the LAST server failed): executed 21 ticks, threw 0
this patch (throw when ANY server failed): executed 1 tick, threw 1
One failover drill stops this client's heartbeats permanently, until the process restarts; admin then ages the URI out and stops routing to it. It also aborts uris.forEach mid-cycle, so every other registered URI loses that beat too.
In fairness the hazard exists on master as well - when the last server fails - but this patch promotes a rare edge case to the common path.
Two acceptable fixes: (a) do not throw from doHeartbeat at all, keeping the accumulation only in doRegister where FailbackRegistryRepository.java:77-82 really does catch it and enqueue a retry; or (b) keep throwing and wrap each URI's heartbeat in try/catch inside the scheduled body, which also fixes the missing per-URI isolation.
|
Re-check: this head has a real failure.
Please fix that test error so the branch can be evaluated on its merits. |
Aias00
left a comment
Aias00
left a comment
There was a problem hiding this comment.
Re-reviewing after the head was updated. pr_build now passes (the earlier HeartbeatFailureIsolationTest.failureDoesNotSuppressOtherUrisOrSubsequentTicks failure is gone) and all shenyu-integrated-test-*, k8s-ingress and e2e jobs are green. The registration-failure propagation logic was already approved by @utafrali. Dismissing my earlier request-changes and approving.
Summary
Fixes #6559
Validation
./mvnw -pl shenyu-register-center/shenyu-register-client/shenyu-register-client-http -am -Dtest=HttpClientRegisterRepositoryTest -DfailIfNoTests=false testRetries still replay registration to all configured servers, including previously successful servers, using the existing registration semantics.