lymerin commented on PR #7450:
URL: https://github.com/apache/shenyu/pull/7450#issuecomment-6122701836
> ## 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.
During that re-check, I found one interaction that was not covered by the
previous version.
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 there was another
full-sync path in `AbstractDiscoveryProcessor.fetchAll()`. That method rebuilt
upstream data from `InstanceEntity`, where `protocol` was defaulted to
`http://` and richer properties were not available.
After this PR started updating existing cached upstream fields such as
`protocol`, `warmup`, and metadata in place, that incomplete full-sync data
could overwrite a configuration that had just been updated correctly by the
Eureka watcher.
In other words:
```yaml
Eureka watcher updates:
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.
This keeps the current behavior for newly discovered instances, but avoids
replacing richer persisted configuration for existing instances with registry
defaults.
I also added focused regression coverage for:
- preserving existing protocol/weight/props during full sync;
- publishing the reconciled result after add/remove;
- publishing an empty list after all missing upstreams are removed.
I also trimmed the assertions so the tests only cover behavior relevant to
this change.
The IP-based Eureka address change is still intentional: full discovery and
watcher events now use the same address form so updates can match the existing
row consistently.
--
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]
