You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shenyu.apache.org by "guanzhenxing (via GitHub)" <gi...@apache.org> on 2026/01/19 08:28:17 UTC
[PR] Fix: Preserve Gateway's independent upstream health check state when receiving config updates from Admin [shenyu]
guanzhenxing opened a new pull request, #6274:
URL: https://github.com/apache/shenyu/pull/6274
## Problem Description
When the Admin side health check detects an upstream as unhealthy and publishes a configuration update with `status=false`, the Gateway side completely removes that upstream from both `healthyUpstream` and `unhealthyUpstream` maps. This causes the Gateway to lose track of the upstream, preventing its independent health check from recovering the upstream when it becomes healthy again.
## Error Manifestation
divide upstream configuration error
CANNOT_FIND_HEALTHY_UPSTREAM_URL
## Root Cause
ShenYu has **two independent health check systems:**
1. **Admin side:** Checks upstream health and publishes configuration updates
2. **Gateway side:** Independently runs health checks and maintains its own health state
The issue occurs when:
1. Admin's health check marks an upstream as unhealthy (`status=false`)
2. Admin publishes a configuration update
3. Gateway receives the update via `DivideUpstreamDataHandler`
4. `UpstreamCacheManager.submit()` processes `status=false` upstreams
5. Bug: Original code calls `triggerRemoveOne()` which removes the upstream from BOTH healthy and unhealthy maps
6. Result: Gateway loses all tracking of this upstream - even if it recovers, Gateway won't know
## Solution
Design Principle
Gateway's health check state should be independent of Admin's configuration updates
Core Logic Change
Before:
status=false → triggerRemoveOne() → completely removed from both maps
After:
status=false AND healthCheckEnabled=true → preserve in unhealthy map → continue health checking
status=false AND healthCheckEnabled=false → remove (no monitoring needed)
Changes
1. UpstreamCacheManager.java
Refactored `submit()` method
- Extracted logic into smaller, focused methods for better maintainability
- Fixed `ConcurrentModificationException` by creating ArrayList copy before iteration
New method: `processOfflineUpstreams()`
Handles upstreams with `status=false`:
```
// If upstream was previously in unhealthy map AND health check is enabled:
// → Keep it in unhealthy map for continued monitoring
// If upstream was not previously unhealthy OR health check is disabled:
// → Remove it (no monitoring needed)
```
New method: `processValidUpstreams()`
Handles upstreams with `status=true`:
- Checks if upstream was previously in `unhealthyUpstream` map
- If yes, preserves the unhealthy state instead of forcing it to healthy
- This allows Gateway's health check to recover it naturally
New method: `getCurrentUnhealthyMap()`
Helper method to get current unhealthy upstreams for state preservation
2. UpstreamCheckTask.java
Made `putToMap()` and `removeFromMap() ` public
- These methods were `private` but are needed by `UpstreamCacheManager`
- Now allows preserving unhealthy state across configuration updates
Testing
Added 9 comprehensive tests to verify the fix:
**UpstreamCacheManagerTest (4 new tests)**
1. `testSubmitWithStatusFalsePreservesUnhealthyState`: Verifies upstreams with `status=false` that were previously unhealthy remain in unhealthy map
2. `testSubmitWithNewOfflineUpstreamAddedToUnhealthy`: Verifies new upstreams with `status=false` are added to unhealthy map for monitoring
3. `testSubmitPreservesUnhealthyForValidUpstream`: Verifies valid upstreams (`status=true`) that were previously unhealthy remain in unhealthy map
4. `testSubmitWithHealthCheckDisabledAndStatusFalse`: Verifies upstreams with `healthCheckEnabled=false` are removed, not added to unhealthy map
**UpstreamCheckTaskTest (5 new tests)**
5. `testPutToMap`: Tests adding upstreams to healthy map
6. `testPutToMapUnhealthy`: Tests adding upstreams to unhealthy map
7. `testRemoveFromMap`: Tests removing upstreams from healthy map
8. `testRemoveFromMapUnhealthy`: Tests removing upstreams from unhealthy map
9. `testMoveUpstreamBetweenMaps`: Tests moving upstreams between healthy and unhealthy maps
Test Results
Tests run: 19, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS
Impact
Before Fix
- Gateway loses unhealthy upstream tracking when Admin publishes updates
- Recovered upstreams cannot be detected by Gateway
- Results in `CANNOT_FIND_HEALTHY_UPSTREAM_URL` errors
After Fix
- Gateway preserves its independent health check state
- Unhealthy upstreams continue to be monitored even after Admin updates
- Gateway can automatically recover upstreams when they become healthy
- No manual intervention required
Commits
- 8a0f9e958 - Fix: Preserve unhealthy upstream state when receiving config updates from admin
- 78822b42 - Test: Add tests for upstream unhealthy state preservation
--
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: notifications-unsubscribe@shenyu.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix: Preserve Gateway's independent upstream health check state when receiving config updates from Admin [shenyu]
Posted by "guanzhenxing (via GitHub)" <gi...@apache.org>.
guanzhenxing commented on code in PR #6274:
URL: https://github.com/apache/shenyu/pull/6274#discussion_r2739576402
##########
shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/cache/UpstreamCacheManager.java:
##########
@@ -146,53 +147,106 @@ public void removeByKey(final String key) {
*/
public void submit(final String selectorId, final List<Upstream> upstreamList) {
List<Upstream> actualUpstreamList = Objects.isNull(upstreamList) ? Lists.newArrayList() : upstreamList;
- actualUpstreamList.forEach(upstream -> {
- if (!upstream.isHealthCheckEnabled()) {
- upstream.setStatus(true);
- upstream.setHealthy(true);
- }
- });
+ initializeUpstreamHealthStatus(actualUpstreamList);
Review Comment:
Done
--
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: notifications-unsubscribe@shenyu.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix: Preserve Gateway's independent upstream health check state when receiving config updates from Admin [shenyu]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 merged PR #6274:
URL: https://github.com/apache/shenyu/pull/6274
--
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: notifications-unsubscribe@shenyu.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix: Preserve Gateway's independent upstream health check state when receiving config updates from Admin [shenyu]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 commented on code in PR #6274:
URL: https://github.com/apache/shenyu/pull/6274#discussion_r2731043524
##########
shenyu-loadbalancer/src/test/java/org/apache/shenyu/loadbalancer/cache/UpstreamCheckTaskTest.java:
##########
@@ -164,4 +164,141 @@ public void testHealthCheckEnabledDefaultsToTrue() {
assertTrue(upstream.isHealthCheckEnabled());
}
+
+ /**
+ * Test public putToMap method.
+ */
+ @Test
+ public void testPutToMap() {
+ final String selectorId = "putToMapTest";
+ Upstream upstream1 = Upstream.builder()
+ .url("upstream1:8080")
+ .build();
+ Upstream upstream2 = Upstream.builder()
+ .url("upstream2:8080")
+ .build();
+
+ // Test adding to healthy map
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream1);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(1));
+
+ // Test adding another upstream
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream2);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(2));
+
+ // Test adding duplicate (should not add again)
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream1);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(2));
+
+ // Clean up
+ healthCheckTask.triggerRemoveAll(selectorId);
+ }
+
+ /**
+ * Test public putToMap method with unhealthy map.
+ */
+ @Test
+ public void testPutToMapUnhealthy() {
+ final String selectorId = "putToMapUnhealthyTest";
+ Upstream upstream = Upstream.builder()
+ .url("unhealthy-upstream:8080")
+ .build();
+
+ // Test adding to unhealthy map
+ healthCheckTask.putToMap(healthCheckTask.getUnhealthyUpstream(), selectorId, upstream);
+ assertThat(healthCheckTask.getUnhealthyUpstream().get(selectorId).size(), is(1));
+
+ // Verify it's not in healthy map
+ assertTrue(!healthCheckTask.getHealthyUpstream().containsKey(selectorId)
+ || healthCheckTask.getHealthyUpstream().get(selectorId).isEmpty());
+
+ // Clean up
+ healthCheckTask.triggerRemoveAll(selectorId);
+ }
+
+ /**
+ * Test public removeFromMap method.
+ */
+ @Test
+ public void testRemoveFromMap() {
+ final String selectorId = "removeFromMapTest";
+ Upstream upstream1 = Upstream.builder()
+ .url("remove1:8080")
+ .build();
+ Upstream upstream2 = Upstream.builder()
+ .url("remove2:8080")
+ .build();
+
+ // Add upstreams to healthy map
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream1);
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream2);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(2));
+
+ // Remove one upstream
+ healthCheckTask.removeFromMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream1);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(1));
+
+ // Verify correct upstream remains
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).get(0).getUrl(), is("remove2:8080"));
+
+ // Clean up
+ healthCheckTask.triggerRemoveAll(selectorId);
+ }
+
+ /**
+ * Test public removeFromMap method with unhealthy map.
+ */
+ @Test
+ public void testRemoveFromMapUnhealthy() {
+ final String selectorId = "removeFromMapUnhealthyTest";
+ Upstream upstream = Upstream.builder()
+ .url("unhealthy-to-remove:8080")
+ .build();
+
+ // Add to unhealthy map
+ healthCheckTask.putToMap(healthCheckTask.getUnhealthyUpstream(), selectorId, upstream);
+ assertThat(healthCheckTask.getUnhealthyUpstream().get(selectorId).size(), is(1));
+
+ // Remove from unhealthy map
+ healthCheckTask.removeFromMap(healthCheckTask.getUnhealthyUpstream(), selectorId, upstream);
+ assertTrue(!healthCheckTask.getUnhealthyUpstream().containsKey(selectorId)
+ || healthCheckTask.getUnhealthyUpstream().get(selectorId).isEmpty());
+ }
+
+ /**
+ * Test moving upstream between healthy and unhealthy maps using public methods.
+ */
+ @Test
+ public void testMoveUpstreamBetweenMaps() {
+ final String selectorId = "moveBetweenMapsTest";
+ Upstream upstream = Upstream.builder()
+ .url("moving-upstream:8080")
+ .build();
+ upstream.setHealthy(true);
+
+ // Start in healthy map
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(1));
+
+ // Move to unhealthy map
+ healthCheckTask.removeFromMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream);
+ healthCheckTask.putToMap(healthCheckTask.getUnhealthyUpstream(), selectorId, upstream);
+
+ // Verify moved
+ assertTrue(!healthCheckTask.getHealthyUpstream().containsKey(selectorId)
+ || healthCheckTask.getHealthyUpstream().get(selectorId).isEmpty());
Review Comment:
@guanzhenxing pls do this
--
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: notifications-unsubscribe@shenyu.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix: Preserve Gateway's independent upstream health check state when receiving config updates from Admin [shenyu]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 commented on code in PR #6274:
URL: https://github.com/apache/shenyu/pull/6274#discussion_r2731034474
##########
shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/cache/UpstreamCacheManager.java:
##########
@@ -146,53 +147,106 @@ public void removeByKey(final String key) {
*/
public void submit(final String selectorId, final List<Upstream> upstreamList) {
List<Upstream> actualUpstreamList = Objects.isNull(upstreamList) ? Lists.newArrayList() : upstreamList;
- actualUpstreamList.forEach(upstream -> {
- if (!upstream.isHealthCheckEnabled()) {
- upstream.setStatus(true);
- upstream.setHealthy(true);
- }
- });
+ initializeUpstreamHealthStatus(actualUpstreamList);
Review Comment:
The `initializeUpstreamHealthStatus` method is called before partitioning the upstreams. This might lead to unnecessary processing if the list is empty.
Consider moving this call after checking if the list is empty to avoid unnecessary processing.
--
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: notifications-unsubscribe@shenyu.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix: Preserve Gateway's independent upstream health check state when receiving config updates from Admin [shenyu]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #6274:
URL: https://github.com/apache/shenyu/pull/6274#discussion_r2706775886
##########
shenyu-loadbalancer/src/test/java/org/apache/shenyu/loadbalancer/cache/UpstreamCheckTaskTest.java:
##########
@@ -164,4 +164,141 @@ public void testHealthCheckEnabledDefaultsToTrue() {
assertTrue(upstream.isHealthCheckEnabled());
}
+
+ /**
+ * Test public putToMap method.
+ */
+ @Test
+ public void testPutToMap() {
+ final String selectorId = "putToMapTest";
+ Upstream upstream1 = Upstream.builder()
+ .url("upstream1:8080")
+ .build();
+ Upstream upstream2 = Upstream.builder()
+ .url("upstream2:8080")
+ .build();
+
+ // Test adding to healthy map
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream1);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(1));
+
+ // Test adding another upstream
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream2);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(2));
+
+ // Test adding duplicate (should not add again)
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream1);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(2));
+
+ // Clean up
+ healthCheckTask.triggerRemoveAll(selectorId);
+ }
+
+ /**
+ * Test public putToMap method with unhealthy map.
+ */
+ @Test
+ public void testPutToMapUnhealthy() {
+ final String selectorId = "putToMapUnhealthyTest";
+ Upstream upstream = Upstream.builder()
+ .url("unhealthy-upstream:8080")
+ .build();
+
+ // Test adding to unhealthy map
+ healthCheckTask.putToMap(healthCheckTask.getUnhealthyUpstream(), selectorId, upstream);
+ assertThat(healthCheckTask.getUnhealthyUpstream().get(selectorId).size(), is(1));
+
+ // Verify it's not in healthy map
+ assertTrue(!healthCheckTask.getHealthyUpstream().containsKey(selectorId)
+ || healthCheckTask.getHealthyUpstream().get(selectorId).isEmpty());
+
+ // Clean up
+ healthCheckTask.triggerRemoveAll(selectorId);
+ }
+
+ /**
+ * Test public removeFromMap method.
+ */
+ @Test
+ public void testRemoveFromMap() {
+ final String selectorId = "removeFromMapTest";
+ Upstream upstream1 = Upstream.builder()
+ .url("remove1:8080")
+ .build();
+ Upstream upstream2 = Upstream.builder()
+ .url("remove2:8080")
+ .build();
+
+ // Add upstreams to healthy map
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream1);
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream2);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(2));
+
+ // Remove one upstream
+ healthCheckTask.removeFromMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream1);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(1));
+
+ // Verify correct upstream remains
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).get(0).getUrl(), is("remove2:8080"));
+
+ // Clean up
+ healthCheckTask.triggerRemoveAll(selectorId);
+ }
+
+ /**
+ * Test public removeFromMap method with unhealthy map.
+ */
+ @Test
+ public void testRemoveFromMapUnhealthy() {
+ final String selectorId = "removeFromMapUnhealthyTest";
+ Upstream upstream = Upstream.builder()
+ .url("unhealthy-to-remove:8080")
+ .build();
+
+ // Add to unhealthy map
+ healthCheckTask.putToMap(healthCheckTask.getUnhealthyUpstream(), selectorId, upstream);
+ assertThat(healthCheckTask.getUnhealthyUpstream().get(selectorId).size(), is(1));
+
+ // Remove from unhealthy map
+ healthCheckTask.removeFromMap(healthCheckTask.getUnhealthyUpstream(), selectorId, upstream);
+ assertTrue(!healthCheckTask.getUnhealthyUpstream().containsKey(selectorId)
+ || healthCheckTask.getUnhealthyUpstream().get(selectorId).isEmpty());
+ }
+
+ /**
+ * Test moving upstream between healthy and unhealthy maps using public methods.
+ */
+ @Test
+ public void testMoveUpstreamBetweenMaps() {
+ final String selectorId = "moveBetweenMapsTest";
+ Upstream upstream = Upstream.builder()
+ .url("moving-upstream:8080")
+ .build();
+ upstream.setHealthy(true);
+
+ // Start in healthy map
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(1));
+
+ // Move to unhealthy map
+ healthCheckTask.removeFromMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream);
+ healthCheckTask.putToMap(healthCheckTask.getUnhealthyUpstream(), selectorId, upstream);
+
+ // Verify moved
+ assertTrue(!healthCheckTask.getHealthyUpstream().containsKey(selectorId)
+ || healthCheckTask.getHealthyUpstream().get(selectorId).isEmpty());
+ assertThat(healthCheckTask.getUnhealthyUpstream().get(selectorId).size(), is(1));
+
+ // Move back to healthy
+ healthCheckTask.removeFromMap(healthCheckTask.getUnhealthyUpstream(), selectorId, upstream);
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream);
+
+ // Verify moved back
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(1));
+ assertTrue(!healthCheckTask.getUnhealthyUpstream().containsKey(selectorId)
+ || healthCheckTask.getUnhealthyUpstream().get(selectorId).isEmpty());
Review Comment:
The assertion logic uses double negation which reduces code readability. Instead of checking `!healthCheckTask.getUnhealthyUpstream().containsKey(selectorId) || healthCheckTask.getUnhealthyUpstream().get(selectorId).isEmpty()`, consider using a more straightforward assertion that directly validates the absence of upstreams in the unhealthy map.
##########
shenyu-loadbalancer/src/test/java/org/apache/shenyu/loadbalancer/cache/UpstreamCheckTaskTest.java:
##########
@@ -164,4 +164,141 @@ public void testHealthCheckEnabledDefaultsToTrue() {
assertTrue(upstream.isHealthCheckEnabled());
}
+
+ /**
+ * Test public putToMap method.
+ */
+ @Test
+ public void testPutToMap() {
+ final String selectorId = "putToMapTest";
+ Upstream upstream1 = Upstream.builder()
+ .url("upstream1:8080")
+ .build();
+ Upstream upstream2 = Upstream.builder()
+ .url("upstream2:8080")
+ .build();
+
+ // Test adding to healthy map
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream1);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(1));
+
+ // Test adding another upstream
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream2);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(2));
+
+ // Test adding duplicate (should not add again)
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream1);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(2));
+
+ // Clean up
+ healthCheckTask.triggerRemoveAll(selectorId);
+ }
+
+ /**
+ * Test public putToMap method with unhealthy map.
+ */
+ @Test
+ public void testPutToMapUnhealthy() {
+ final String selectorId = "putToMapUnhealthyTest";
+ Upstream upstream = Upstream.builder()
+ .url("unhealthy-upstream:8080")
+ .build();
+
+ // Test adding to unhealthy map
+ healthCheckTask.putToMap(healthCheckTask.getUnhealthyUpstream(), selectorId, upstream);
+ assertThat(healthCheckTask.getUnhealthyUpstream().get(selectorId).size(), is(1));
+
+ // Verify it's not in healthy map
+ assertTrue(!healthCheckTask.getHealthyUpstream().containsKey(selectorId)
+ || healthCheckTask.getHealthyUpstream().get(selectorId).isEmpty());
+
+ // Clean up
+ healthCheckTask.triggerRemoveAll(selectorId);
+ }
+
+ /**
+ * Test public removeFromMap method.
+ */
+ @Test
+ public void testRemoveFromMap() {
+ final String selectorId = "removeFromMapTest";
+ Upstream upstream1 = Upstream.builder()
+ .url("remove1:8080")
+ .build();
+ Upstream upstream2 = Upstream.builder()
+ .url("remove2:8080")
+ .build();
+
+ // Add upstreams to healthy map
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream1);
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream2);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(2));
+
+ // Remove one upstream
+ healthCheckTask.removeFromMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream1);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(1));
+
+ // Verify correct upstream remains
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).get(0).getUrl(), is("remove2:8080"));
+
+ // Clean up
+ healthCheckTask.triggerRemoveAll(selectorId);
+ }
+
+ /**
+ * Test public removeFromMap method with unhealthy map.
+ */
+ @Test
+ public void testRemoveFromMapUnhealthy() {
+ final String selectorId = "removeFromMapUnhealthyTest";
+ Upstream upstream = Upstream.builder()
+ .url("unhealthy-to-remove:8080")
+ .build();
+
+ // Add to unhealthy map
+ healthCheckTask.putToMap(healthCheckTask.getUnhealthyUpstream(), selectorId, upstream);
+ assertThat(healthCheckTask.getUnhealthyUpstream().get(selectorId).size(), is(1));
+
+ // Remove from unhealthy map
+ healthCheckTask.removeFromMap(healthCheckTask.getUnhealthyUpstream(), selectorId, upstream);
+ assertTrue(!healthCheckTask.getUnhealthyUpstream().containsKey(selectorId)
+ || healthCheckTask.getUnhealthyUpstream().get(selectorId).isEmpty());
+ }
+
+ /**
+ * Test moving upstream between healthy and unhealthy maps using public methods.
+ */
+ @Test
+ public void testMoveUpstreamBetweenMaps() {
+ final String selectorId = "moveBetweenMapsTest";
+ Upstream upstream = Upstream.builder()
+ .url("moving-upstream:8080")
+ .build();
+ upstream.setHealthy(true);
+
+ // Start in healthy map
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(1));
+
+ // Move to unhealthy map
+ healthCheckTask.removeFromMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream);
+ healthCheckTask.putToMap(healthCheckTask.getUnhealthyUpstream(), selectorId, upstream);
+
+ // Verify moved
+ assertTrue(!healthCheckTask.getHealthyUpstream().containsKey(selectorId)
+ || healthCheckTask.getHealthyUpstream().get(selectorId).isEmpty());
Review Comment:
The assertion logic uses double negation which reduces code readability. Instead of checking `!healthCheckTask.getHealthyUpstream().containsKey(selectorId) || healthCheckTask.getHealthyUpstream().get(selectorId).isEmpty()`, consider using a more straightforward assertion that directly validates the absence of upstreams in the healthy map.
##########
shenyu-loadbalancer/src/test/java/org/apache/shenyu/loadbalancer/cache/UpstreamCheckTaskTest.java:
##########
@@ -164,4 +164,141 @@ public void testHealthCheckEnabledDefaultsToTrue() {
assertTrue(upstream.isHealthCheckEnabled());
}
+
+ /**
+ * Test public putToMap method.
+ */
+ @Test
+ public void testPutToMap() {
+ final String selectorId = "putToMapTest";
+ Upstream upstream1 = Upstream.builder()
+ .url("upstream1:8080")
+ .build();
+ Upstream upstream2 = Upstream.builder()
+ .url("upstream2:8080")
+ .build();
+
+ // Test adding to healthy map
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream1);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(1));
+
+ // Test adding another upstream
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream2);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(2));
+
+ // Test adding duplicate (should not add again)
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream1);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(2));
+
+ // Clean up
+ healthCheckTask.triggerRemoveAll(selectorId);
+ }
+
+ /**
+ * Test public putToMap method with unhealthy map.
+ */
+ @Test
+ public void testPutToMapUnhealthy() {
+ final String selectorId = "putToMapUnhealthyTest";
+ Upstream upstream = Upstream.builder()
+ .url("unhealthy-upstream:8080")
+ .build();
+
+ // Test adding to unhealthy map
+ healthCheckTask.putToMap(healthCheckTask.getUnhealthyUpstream(), selectorId, upstream);
+ assertThat(healthCheckTask.getUnhealthyUpstream().get(selectorId).size(), is(1));
+
+ // Verify it's not in healthy map
+ assertTrue(!healthCheckTask.getHealthyUpstream().containsKey(selectorId)
+ || healthCheckTask.getHealthyUpstream().get(selectorId).isEmpty());
+
+ // Clean up
+ healthCheckTask.triggerRemoveAll(selectorId);
+ }
+
+ /**
+ * Test public removeFromMap method.
+ */
+ @Test
+ public void testRemoveFromMap() {
+ final String selectorId = "removeFromMapTest";
+ Upstream upstream1 = Upstream.builder()
+ .url("remove1:8080")
+ .build();
+ Upstream upstream2 = Upstream.builder()
+ .url("remove2:8080")
+ .build();
+
+ // Add upstreams to healthy map
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream1);
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream2);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(2));
+
+ // Remove one upstream
+ healthCheckTask.removeFromMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream1);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(1));
+
+ // Verify correct upstream remains
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).get(0).getUrl(), is("remove2:8080"));
+
+ // Clean up
+ healthCheckTask.triggerRemoveAll(selectorId);
+ }
+
+ /**
+ * Test public removeFromMap method with unhealthy map.
+ */
+ @Test
+ public void testRemoveFromMapUnhealthy() {
+ final String selectorId = "removeFromMapUnhealthyTest";
+ Upstream upstream = Upstream.builder()
+ .url("unhealthy-to-remove:8080")
+ .build();
+
+ // Add to unhealthy map
+ healthCheckTask.putToMap(healthCheckTask.getUnhealthyUpstream(), selectorId, upstream);
+ assertThat(healthCheckTask.getUnhealthyUpstream().get(selectorId).size(), is(1));
+
+ // Remove from unhealthy map
+ healthCheckTask.removeFromMap(healthCheckTask.getUnhealthyUpstream(), selectorId, upstream);
+ assertTrue(!healthCheckTask.getUnhealthyUpstream().containsKey(selectorId)
+ || healthCheckTask.getUnhealthyUpstream().get(selectorId).isEmpty());
Review Comment:
The assertion logic uses double negation which reduces code readability. Instead of checking `!healthCheckTask.getUnhealthyUpstream().containsKey(selectorId) || healthCheckTask.getUnhealthyUpstream().get(selectorId).isEmpty()`, consider using a more straightforward assertion that directly validates the absence of upstreams in the unhealthy map.
##########
shenyu-loadbalancer/src/test/java/org/apache/shenyu/loadbalancer/cache/UpstreamCheckTaskTest.java:
##########
@@ -164,4 +164,141 @@ public void testHealthCheckEnabledDefaultsToTrue() {
assertTrue(upstream.isHealthCheckEnabled());
}
+
+ /**
+ * Test public putToMap method.
+ */
+ @Test
+ public void testPutToMap() {
+ final String selectorId = "putToMapTest";
+ Upstream upstream1 = Upstream.builder()
+ .url("upstream1:8080")
+ .build();
+ Upstream upstream2 = Upstream.builder()
+ .url("upstream2:8080")
+ .build();
+
+ // Test adding to healthy map
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream1);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(1));
+
+ // Test adding another upstream
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream2);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(2));
+
+ // Test adding duplicate (should not add again)
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream1);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(2));
+
+ // Clean up
+ healthCheckTask.triggerRemoveAll(selectorId);
+ }
+
+ /**
+ * Test public putToMap method with unhealthy map.
+ */
+ @Test
+ public void testPutToMapUnhealthy() {
+ final String selectorId = "putToMapUnhealthyTest";
+ Upstream upstream = Upstream.builder()
+ .url("unhealthy-upstream:8080")
+ .build();
+
+ // Test adding to unhealthy map
+ healthCheckTask.putToMap(healthCheckTask.getUnhealthyUpstream(), selectorId, upstream);
+ assertThat(healthCheckTask.getUnhealthyUpstream().get(selectorId).size(), is(1));
+
+ // Verify it's not in healthy map
+ assertTrue(!healthCheckTask.getHealthyUpstream().containsKey(selectorId)
+ || healthCheckTask.getHealthyUpstream().get(selectorId).isEmpty());
Review Comment:
The assertion logic uses double negation which reduces code readability. Instead of checking `!healthCheckTask.getHealthyUpstream().containsKey(selectorId) || healthCheckTask.getHealthyUpstream().get(selectorId).isEmpty()`, consider using a more straightforward assertion that directly validates the absence of upstreams in the healthy map.
```suggestion
assertTrue(CollectionUtils.isEmpty(healthCheckTask.getHealthyUpstream().get(selectorId)));
```
--
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: notifications-unsubscribe@shenyu.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix: Preserve Gateway's independent upstream health check state when receiving config updates from Admin [shenyu]
Posted by "guanzhenxing (via GitHub)" <gi...@apache.org>.
guanzhenxing commented on PR #6274:
URL: https://github.com/apache/shenyu/pull/6274#issuecomment-3803380937
@Aias00 这个pull request还有需要改哪些？
--
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: notifications-unsubscribe@shenyu.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix: Preserve Gateway's independent upstream health check state when receiving config updates from Admin [shenyu]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 commented on code in PR #6274:
URL: https://github.com/apache/shenyu/pull/6274#discussion_r2731044534
##########
shenyu-loadbalancer/src/test/java/org/apache/shenyu/loadbalancer/cache/UpstreamCheckTaskTest.java:
##########
@@ -164,4 +164,141 @@ public void testHealthCheckEnabledDefaultsToTrue() {
assertTrue(upstream.isHealthCheckEnabled());
}
+
+ /**
+ * Test public putToMap method.
+ */
+ @Test
+ public void testPutToMap() {
+ final String selectorId = "putToMapTest";
+ Upstream upstream1 = Upstream.builder()
+ .url("upstream1:8080")
+ .build();
+ Upstream upstream2 = Upstream.builder()
+ .url("upstream2:8080")
+ .build();
+
+ // Test adding to healthy map
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream1);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(1));
+
+ // Test adding another upstream
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream2);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(2));
+
+ // Test adding duplicate (should not add again)
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream1);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(2));
+
+ // Clean up
+ healthCheckTask.triggerRemoveAll(selectorId);
+ }
+
+ /**
+ * Test public putToMap method with unhealthy map.
+ */
+ @Test
+ public void testPutToMapUnhealthy() {
+ final String selectorId = "putToMapUnhealthyTest";
+ Upstream upstream = Upstream.builder()
+ .url("unhealthy-upstream:8080")
+ .build();
+
+ // Test adding to unhealthy map
+ healthCheckTask.putToMap(healthCheckTask.getUnhealthyUpstream(), selectorId, upstream);
+ assertThat(healthCheckTask.getUnhealthyUpstream().get(selectorId).size(), is(1));
+
+ // Verify it's not in healthy map
+ assertTrue(!healthCheckTask.getHealthyUpstream().containsKey(selectorId)
+ || healthCheckTask.getHealthyUpstream().get(selectorId).isEmpty());
+
+ // Clean up
+ healthCheckTask.triggerRemoveAll(selectorId);
+ }
+
+ /**
+ * Test public removeFromMap method.
+ */
+ @Test
+ public void testRemoveFromMap() {
+ final String selectorId = "removeFromMapTest";
+ Upstream upstream1 = Upstream.builder()
+ .url("remove1:8080")
+ .build();
+ Upstream upstream2 = Upstream.builder()
+ .url("remove2:8080")
+ .build();
+
+ // Add upstreams to healthy map
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream1);
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream2);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(2));
+
+ // Remove one upstream
+ healthCheckTask.removeFromMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream1);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(1));
+
+ // Verify correct upstream remains
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).get(0).getUrl(), is("remove2:8080"));
+
+ // Clean up
+ healthCheckTask.triggerRemoveAll(selectorId);
+ }
+
+ /**
+ * Test public removeFromMap method with unhealthy map.
+ */
+ @Test
+ public void testRemoveFromMapUnhealthy() {
+ final String selectorId = "removeFromMapUnhealthyTest";
+ Upstream upstream = Upstream.builder()
+ .url("unhealthy-to-remove:8080")
+ .build();
+
+ // Add to unhealthy map
+ healthCheckTask.putToMap(healthCheckTask.getUnhealthyUpstream(), selectorId, upstream);
+ assertThat(healthCheckTask.getUnhealthyUpstream().get(selectorId).size(), is(1));
+
+ // Remove from unhealthy map
+ healthCheckTask.removeFromMap(healthCheckTask.getUnhealthyUpstream(), selectorId, upstream);
+ assertTrue(!healthCheckTask.getUnhealthyUpstream().containsKey(selectorId)
+ || healthCheckTask.getUnhealthyUpstream().get(selectorId).isEmpty());
+ }
+
+ /**
+ * Test moving upstream between healthy and unhealthy maps using public methods.
+ */
+ @Test
+ public void testMoveUpstreamBetweenMaps() {
+ final String selectorId = "moveBetweenMapsTest";
+ Upstream upstream = Upstream.builder()
+ .url("moving-upstream:8080")
+ .build();
+ upstream.setHealthy(true);
+
+ // Start in healthy map
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream);
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(1));
+
+ // Move to unhealthy map
+ healthCheckTask.removeFromMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream);
+ healthCheckTask.putToMap(healthCheckTask.getUnhealthyUpstream(), selectorId, upstream);
+
+ // Verify moved
+ assertTrue(!healthCheckTask.getHealthyUpstream().containsKey(selectorId)
+ || healthCheckTask.getHealthyUpstream().get(selectorId).isEmpty());
+ assertThat(healthCheckTask.getUnhealthyUpstream().get(selectorId).size(), is(1));
+
+ // Move back to healthy
+ healthCheckTask.removeFromMap(healthCheckTask.getUnhealthyUpstream(), selectorId, upstream);
+ healthCheckTask.putToMap(healthCheckTask.getHealthyUpstream(), selectorId, upstream);
+
+ // Verify moved back
+ assertThat(healthCheckTask.getHealthyUpstream().get(selectorId).size(), is(1));
+ assertTrue(!healthCheckTask.getUnhealthyUpstream().containsKey(selectorId)
+ || healthCheckTask.getUnhealthyUpstream().get(selectorId).isEmpty());
Review Comment:
@guanzhenxing pls do this
--
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: notifications-unsubscribe@shenyu.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org