[lymerin]

Fixes #7439.

Problem

The UPDATED predicate required matching instanceId values and !InstanceInfo.equals(). Since Eureka equality compares instance identity, configuration changes on the same instance never satisfied that predicate. The add/delete checks also treated the instance as unchanged, so no event was emitted.

Impact

Changes to weight, protocol, status, or props left upstream configuration stale despite continued Eureka polling. The full-sync path also published registry defaults for existing records, conflicting with the configuration refresh needed in the gateway cache.

Changes

• Pair Eureka snapshots by instanceId. For an unchanged IP and port, compare the generated upstream payload and emit UPDATED only when that payload changes. Unrelated metadata remains a no-op.
• For an IP or port change, emit ADDED for the new endpoint before DELETED for the old endpoint, with no UPDATED event.
• Use IP addresses consistently in full discovery and watcher events. ShenYu SDK registration already populates hostname and IP from the same host value, so its effective address is unchanged.
• Publish persisted upstream configuration after full-sync URL reconciliation, preserving existing protocol, weight, and props. This shared path also applies to Nacos, ZooKeeper, and Etcd; new-instance initialization remains unchanged.
• Refresh existing cached upstream configuration in place for Divide and WebSocket, retaining runtime objects and keeping protocol-derived caches consistent. Preserve existing explicit add/delete and multiple-protocol cache semantics.
• Add focused regression coverage for Eureka events, full-sync configuration preservation, and Divide/WebSocket configuration refresh.

Validation

Targeted Java 17 regression tests and Checkstyle passed. After the final event-order adjustment, all 13 Eureka regression tests and Checkstyle passed again. The full clean install with tests passed locally.

Non-blocking note

Address replacement is limited to Eureka event classification and the existing Admin add/delete path. Gateway updates still merge non-empty upstream lists; removing an endpoint from the persisted snapshot does not itself evict that endpoint from the gateway cache. Endpoint lifecycle reconciliation is outside this PR.

Checklist

Make sure that:

• You have read the contribution guidelines.
• You submit test cases (unit or integration tests) that back your changes.
• Your local test passed ./mvnw clean install -Dmaven.javadoc.skip=true.

[Aias00]

Reviewed current head 9e98e8594628.

Eureka metadata/address changes now publish updates and the gateway cache updates existing same-instance upstreams without retaining stale protocol/config data.

Architecture note: Eureka upstream changes emit UPDATED and update gateway cached fields; unrelated Eureka metadata is intentionally ignored.

Validation: exact-head targeted Maven reactor tests passed on Java 17.

./mvnw -pl shenyu-admin,shenyu-loadbalancer,shenyu-plugin/shenyu-plugin-proxy/shenyu-plugin-divide,shenyu-registry/shenyu-registry-eureka -am test -Dtest=DefaultDiscoveryProcessorTest,DiscoveryDataChangedEventSyncListenerTest,DiscoveryUpstreamMapperTest,UpstreamCacheManagerTest,DivideUpstreamDataHandlerTest,EurekaInstanceRegisterRepositoryTest -Dsurefire.failIfNoSpecifiedTests=false -Drat.skip=true -Djacoco.skip=true -DskipRemoteResources=true -Dmaven.javadoc.skip=true

[Aias00]

Summary

Strong fix for several real bugs:

1. discovery-upstream-sqlmap.xml: WHERE discovery_handler_id = #{id} → #{discoveryHandlerId}. The old #{id} parameter was wrong (and null for fresh rows, per testUpdatedFreshUpstreamSetsHandlerAndTimestampWithoutDatabaseId), so the UPDATE matched 0 rows and never persisted the discovery handler id/url. Important correctness fix.
2. UpstreamCacheManager.submitDiscovery preserves runtime state (protocol, warmup, gray, metadata, inflight, health) when discovery pushes config updates, instead of replacing the object. updateExistingUpstreams now also propagates config to unhealthy instances without recovering them.
3. Eureka compareInstances compares the built upstream JSON instead of the unreliable InstanceInfo.equals, and getInstances uses getIPAddr() for deterministic upstream addresses.

Tests are thorough. Approving.

Non-blocking

• Upstream.protocol / warmup are now mutable (with setters) to allow in-place config refresh. This is consistent with the already-mutable weight/metadata fields, but since Upstream is a widely shared value object, a short comment on the intentional mutability / visibility would help future readers.
• getHostName() → getIPAddr() is a behavior change for Eureka-sourced upstreams. Please confirm it is intended and (if needed) call it out in the PR description for operators who relied on hostnames.

[lymerin]

Summary

Strong fix for several real bugs:

1. discovery-upstream-sqlmap.xml: WHERE discovery_handler_id = #{id} → #{discoveryHandlerId}. The old #{id} parameter was wrong (and null for fresh rows, per testUpdatedFreshUpstreamSetsHandlerAndTimestampWithoutDatabaseId), so the UPDATE matched 0 rows and never persisted the discovery handler id/url. Important correctness fix.
2. UpstreamCacheManager.submitDiscovery preserves runtime state (protocol, warmup, gray, metadata, inflight, health) when discovery pushes config updates, instead of replacing the object. updateExistingUpstreams now also propagates config to unhealthy instances without recovering them.
3. Eureka compareInstances compares the built upstream JSON instead of the unreliable InstanceInfo.equals, and getInstances uses getIPAddr() for deterministic upstream addresses.

Tests are thorough. Approving.

Non-blocking

• Upstream.protocol / warmup are now mutable (with setters) to allow in-place config refresh. This is consistent with the already-mutable weight/metadata fields, but since Upstream is a widely shared value object, a short comment on the intentional mutability / visibility would help future readers.
• getHostName() → getIPAddr() is a behavior change for Eureka-sourced upstreams. Please confirm it is intended and (if needed) call it out in the PR description for operators who relied on hostnames.

Thanks for the review. I moved the PR back to draft because I wanted to re-check the full update flow before asking for another review.

After the re-check, I made one additional fix and also confirmed the intent behind the two non-blocking points you mentioned.

1. protocol / warmup mutability is intentional

The gateway now updates configuration on the existing Upstream instance instead of replacing the object.

This is intentional because replacing the object would also discard runtime state such as inflight counters and timestamps. The mutable fields are therefore treated as configuration that can be refreshed while the runtime object remains the same.

2. The Eureka IP-based address change is also intentional

Full discovery and watcher events previously used different address forms, so the same instance could be stored with one URL and later updated with another.

Using the IP consistently makes both paths refer to the same upstream row. For ShenYu SDK registration, hostname and IP are already populated from the same host value, so this does not change the effective address there.

3. Re-checking the full flow exposed another interaction

The original issue is about incremental Eureka updates: when the same instance changes weight, protocol, or props, the watcher should emit UPDATED, persist the new values, and refresh the existing gateway upstream.

The previous version already fixed that path, but AbstractDiscoveryProcessor.fetchAll() had another full-sync path that rebuilt upstream data from InstanceEntity.

In that path, protocol was defaulted to http://, and richer properties were not available. After this PR started refreshing fields such as protocol, warmup, and metadata on existing cached upstreams, that incomplete full-sync data could overwrite a configuration that had just been updated correctly by the watcher.

Eureka watcher:
https:// + new props
        ↓
Admin / gateway become correct
        ↓
later fetchAll()
        ↓
http:// + default/empty props
        ↓
gateway becomes stale again

I changed fetchAll() so that registry data is used to reconcile which upstream URLs exist, while the final data published to the gateway comes from the persisted discovery_upstream records after that reconciliation.

Since fetchAll() is shared, this also affects Nacos, ZooKeeper, and Etcd: existing upstreams now publish their persisted configuration after reconciliation, while URL reconciliation and new-instance initialization remain unchanged.

4. Regression coverage

I added focused regression coverage for:

• preserving existing protocol, weight, and props during full sync;
• publishing the reconciled result after add/remove;
• publishing an empty list after all missing upstreams are removed.

I also trimmed unrelated assertions so the tests stay focused on this behavior.

[Aias00]

Re-approving. My earlier approval was auto-dismissed (branch protection re-requests review after the new commits), so I re-reviewed the current diff.

This Eureka / discovery upstream sync fix is solid and the new commits added substantial, meaningful tests:

• discovery-upstream-sqlmap.xml: the buggy WHERE discovery_handler_id=#{id} is corrected to #{discoveryHandlerId}, so the update actually persists (previously getId() being null matched zero rows).
• AbstractDiscoveryProcessor.fetchAll now publishes the persisted configuration for existing upstreams instead of registry defaults.
• DiscoveryDataChangedEventSyncListener sets dateUpdated on reconcile updates.
• UpstreamCacheManager.submitDiscovery preserves runtime state (e.g. inflight, health, gray) when a protocol changes, and Upstream now exposes mutable protocol/warmup (with setters) so in-place protocol updates work; updateExistingUpstreams additionally propagates warmup/gray/metadata.
• EurekaInstanceRegisterRepository: compareInstances now compares the upstream JSON (not bare equals()), and getHostName() → getIPAddr() fixes the address used for the upstream.
• Divide/WebSocket handlers now call submitDiscovery.

Tests are strong: DefaultDiscoveryProcessorTest (3 new reconciliation scenarios), UpstreamCacheManagerTest (config update preserves runtime state, unhealthy not recovered, explicit protocol deletion, distinct protocols at same URL), EurekaInstanceRegisterRepositoryTest (parameterized metadata-change publishing, no-op on unchanged/unrelated metadata, add+delete on different instance ids), plus a DiscoveryUpstreamMapperTest integration case.

The earlier blocker (no real effect) is resolved and the code is correct. Approving.

[Aias00]

Heads-up: this branch now has merge conflicts with master (mergeStateStatus: DIRTY), so CI cannot validate it against the current base.

The conflict spans the discovery/upstream path you touch:
DiscoveryDataChangedEventSyncListener.java, AbstractDiscoveryProcessor.java, discovery-upstream-sqlmap.xml, UpstreamCacheManager.java, Upstream.java.

Please rebase onto master — the discovery upstream reconciliation and the loadbalancer cache both changed in that window, so please re-verify the "sync upstream changes for existing instances" behaviour against the current code rather than resolving mechanically. I will re-review once pushed.

[lymerin]

Heads-up: this branch now has merge conflicts with master (mergeStateStatus: DIRTY), so CI cannot validate it against the current base.

The conflict spans the discovery/upstream path you touch: DiscoveryDataChangedEventSyncListener.java, AbstractDiscoveryProcessor.java, discovery-upstream-sqlmap.xml, UpstreamCacheManager.java, Upstream.java.

Please rebase onto master — the discovery upstream reconciliation and the loadbalancer cache both changed in that window, so please re-verify the "sync upstream changes for existing instances" behaviour against the current code rather than resolving mechanically. I will re-review once pushed.

The HTTP integration failure is unrelated to the Eureka changes in this PR. #7903 changed the cryptor response plugin to return a configuration error when a configured response field is missing, while CryptorResponsePluginTest.testWhenDoesntExistFiled still expects the previous behavior and asserts the original response.

The failing run therefore receives the cryptor configuration-error response, which has no userId, causing the assertion to fail with expected "10001" but was null.

The #7903 change is already present in this PR's base branch, and this PR modifies neither the cryptor response plugin nor the failing integration test.

[Aias00]

Approved. This is a coherent fix for Eureka discovery-upstream sync: AbstractDiscoveryProcessor.fetchAll now publishes the persisted upstream configuration (not registry defaults) for existing upstreams, UpstreamCacheManager.submitDiscovery preserves the cached instance when only the protocol changes for a URL (and processValidUpstreams now also reconciles the unhealthy pool and propagates warmup/gray onto existing entries), Upstream.setProtocol correctly refreshes protocolPrefix/selectorKey/identity, and EurekaInstanceRegisterRepository.compareInstances is rewritten to diff by instance id (IP/port change becomes delete+add, content change becomes UPDATE) and use getIPAddr() for addressing. The relaxed immutability on Upstream is contained and well defined.

Two merge notes: (1) the PR currently shows mergeable=false (a merge conflict) and must be rebased/resolved before it can be merged; (2) it edits Upstream.java and UpstreamCacheManager.java, which also overlap with the already-approved #8101 (instance draining) - please rebase on top of that work so the two changes land cleanly. (The shenyu-integrated-test-http failure is the known-flaky http integration job, unrelated to this change.)

[Aias00]

Hi @lymerin, this PR was approved, but the squash-merge could not be completed because it now has merge conflicts (mergeable_state: dirty).

This PR and #8101 (draining admission) both modify Upstream.java / UpstreamCacheManager.java; #8101 was just merged into master ahead of this one. Your branch 7439 therefore conflicts with the merged draining logic.

Please rebase your branch onto the latest master and resolve the conflicts in those files (the draining logic from #8101 is already present), then let CI re-run. Once green, we can squash-merge. Thanks!

[Aias00]

Reviewed Eureka updates for existing instances and propagation through Admin discovery events into gateway upstream caches. No actionable finding.

Validation: 43 focused Admin/Eureka tests passed; an additional gateway/load-balancer/Eureka reactor run passed 66 tests (including repeated coverage). The PR currently has merge conflicts; conflict resolution needs revalidation. Apache RAT passed locally. Scope: source review and targeted verification; full integration/e2e suites were not run locally for this review.