utafrali commented on code in PR #7260:
URL: https://github.com/apache/shenyu/pull/7260#discussion_r4095862454
##########
shenyu-register-center/shenyu-register-client/shenyu-register-client-http/src/test/java/org/apache/shenyu/register/client/http/HttpClientRegisterRepositoryTest.java:
##########
@@ -85,6 +87,54 @@ public void persistUriShouldRegisterToEveryServer() {
}
}
Review 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.
```java
// change to match the surrounding class style
public void partialRegistrationFailureMustPropagate(final String
failedServer) throws IOException {
```
##########
shenyu-register-center/shenyu-register-client/shenyu-register-client-http/src/test/java/org/apache/shenyu/register/client/http/HttpClientRegisterRepositoryTest.java:
##########
@@ -85,6 +87,54 @@ public void persistUriShouldRegisterToEveryServer() {
}
}
+ @ParameterizedTest
+ @ValueSource(strings = {FIRST_SERVER, SECOND_SERVER})
+ void partialRegistrationFailureMustPropagate(final String failedServer)
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.doRegister(anyString(),
eq(failedServer + Constants.URI_PATH), anyString(), anyString()))
+ .thenThrow(new IOException("unavailable"));
+ assertThrows(RuntimeException.class, () ->
multiServerRepository.doPersistURI(uriRegisterDTO()));
+ for (String server : new String[]{FIRST_SERVER, SECOND_SERVER}) {
+ registerUtils.verify(() ->
RegisterUtils.doRegister(anyString(), eq(server + Constants.URI_PATH),
eq(Constants.URI), eq(TOKEN)));
+ }
+ }
+ }
+
+ @ParameterizedTest
+ @ValueSource(strings = {FIRST_SERVER, SECOND_SERVER})
+ void partialHeartbeatFailureMustPropagate(final String failedServer)
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(),
eq(failedServer + Constants.URI_PATH), anyString(), anyString()))
+ .thenThrow(new IOException("unavailable"));
+ assertThrows(RuntimeException.class, () ->
multiServerRepository.sendHeartbeat(uriRegisterDTO()));
+ for (String server : new String[]{FIRST_SERVER, SECOND_SERVER}) {
+ registerUtils.verify(() ->
RegisterUtils.doHeartBeat(anyString(), eq(server + Constants.URI_PATH),
eq(Constants.HEARTBEAT), eq(TOKEN)));
+ }
+ }
+ }
Review 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:
```java
@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);
}
}
```
##########
shenyu-register-center/shenyu-register-client/shenyu-register-client-http/src/test/java/org/apache/shenyu/register/client/http/HttpClientRegisterRepositoryTest.java:
##########
@@ -85,6 +87,54 @@ public void persistUriShouldRegisterToEveryServer() {
}
}
+ @ParameterizedTest
+ @ValueSource(strings = {FIRST_SERVER, SECOND_SERVER})
+ void partialRegistrationFailureMustPropagate(final String failedServer)
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.doRegister(anyString(),
eq(failedServer + Constants.URI_PATH), anyString(), anyString()))
+ .thenThrow(new IOException("unavailable"));
+ assertThrows(RuntimeException.class, () ->
multiServerRepository.doPersistURI(uriRegisterDTO()));
+ for (String server : new String[]{FIRST_SERVER, SECOND_SERVER}) {
+ registerUtils.verify(() ->
RegisterUtils.doRegister(anyString(), eq(server + Constants.URI_PATH),
eq(Constants.URI), eq(TOKEN)));
+ }
+ }
+ }
+
+ @ParameterizedTest
Review 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.
##########
shenyu-register-center/shenyu-register-client/shenyu-register-client-http/src/main/java/org/apache/shenyu/register/client/http/HttpClientRegisterRepository.java:
##########
@@ -218,17 +217,21 @@ private <T> void doRegister(final T t, final String path,
final String type) {
// considering the situation of multiple clusters, we should
continue to execute here
} catch (Exception e) {
LOGGER.error("Register admin url :{} is fail, will retry.
cause:{}", server, e.getMessage());
- if (i == serverList.size()) {
- throw new RuntimeException(e);
+ if (Objects.isNull(failure)) {
+ failure = new RuntimeException(e);
+ } else {
+ failure.addSuppressed(e);
}
}
Review 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:
```java
// 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;
}
```
--
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]
