Repository navigation
enhance: resolve global cluster topology via DNS SRV records - #2099
huanghaoyuanhhy wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: huanghaoyuanhhy The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again. |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (65.50%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #2099 +/- ##
=========================================
Coverage ? 80.83%
=========================================
Files ? 491
Lines ? 30241
Branches ? 3159
=========================================
Hits ? 24444
Misses ? 4470
Partials ? 1327
🚀 New features to boost your workflow:
|
58251e3 to
7523df2
Compare
|
The only failure in the first CI run was This test connects to the localhost Docker standalone, so it does not exercise the SRV discovery path touched by this PR (that path only activates for URIs containing |
Switch global cluster topology discovery from hitting the global endpoint directly to SRV-based ha-manager seed discovery, mirroring pymilvus#3753: - Resolve _grpc._tcp.<hostname> to the ha-manager seed list via the JDK's JNDI DNS provider (no new dependency). Missing SRV records raise instead of falling back to the legacy direct-endpoint path. - Probe seeds concurrently: at least two (one weighted pick per SRV priority, uniform when all weights are zero) on the first fetch, where the first answer wins and the remaining seeds keep being polled in a background watcher that triggers the topology replacement flow if a higher version arrives. Refreshes with a cached version poll every seed and take the highest version, so partial replication cannot regress the cache. - Enforce strict no-rollback version semantics: GlobalStub.onTopologyChange is now a compare-and-set that every update path funnels through, and TopologyRefresher reads the version from the shared topology instead of a private snapshot. - Retry transient DNS failures and unreachable seeds with exponential backoff (3 attempts); deterministic rejections (no SRV records, server-side API errors via the new GlobalClusterApiException) fail fast. - Pass the endpoint hostname verbatim via ?endpoint=; the server derives the global cluster id from its first DNS label. Signed-off-by: huanghaoyuanhhy <haoyuan.huang@zilliz.com>
|
Second run: |
7523df2 to
32f0d9e
Compare
| topology.getVersion(), this::onTopologyChange); | ||
| this.refresher.start(); | ||
| GlobalTopology first = GlobalClusterUtils.fetchTopology(globalEndpoint, authorization, | ||
| null, this::onTopologyChange); |
There was a problem hiding this comment.
sdk-core/src/main/java/io/milvus/v2/client/globalcluster/GlobalStub.java line:70
Medium ---- Wiring the background watcher into this constructor means onTopologyChange can run the primary-swap path before construction finishes: the watcher creates the initial client with clientFactory.create(endpoint, null, true) and then unconditionally calls newClient.startTelemetry() (line 273), bypassing the constructor path's originalConfig.takeTelemetryRuntimeState() / originalConfig.isDeferTelemetryStart() semantics and invoking onPrimaryChange (MilvusClientV2.updatePrimaryConnection) while MilvusClientV2.connect() is still on the stack. Is that interleaving intentional? When a user sets deferTelemetryStart(true), a watcher win silently starts telemetry anyway, and the runtime state seeded in the original config is never consumed.
| // on the server side, keep the cached topology. | ||
| return null; | ||
| } | ||
| if (outcome.apiError != null) { |
There was a problem hiding this comment.
sdk-core/src/main/java/io/milvus/v2/client/globalcluster/GlobalClusterUtils.java line:188
Medium ---- probeCandidates records the last GlobalClusterApiException from any seed, and this line throws it without retry even when the other seeds only failed transiently (IOException). The assumption that any non-zero topology code is identical across all seeds holds for auth, but a single misconfigured/transient seed returning HTTP 200 + non-zero code now converts an otherwise retryable situation into a hard failure. Consider retrying when other seeds reported transient errors and only fail fast when every reachable seed surfaces the same API-level error.
| * Lower {@code priority} is nearer; {@code weight} breaks ties between equal priorities. | ||
| */ | ||
| final class SrvTarget { | ||
| final int priority; |
There was a problem hiding this comment.
sdk-core/src/main/java/io/milvus/v2/client/globalcluster/SrvTarget.java line:28
Low ---- SrvTarget has no equals/hashCode, so the dedup in pickCandidates (picked.contains(target)) relies on reference identity. If SRV returns multiple records pointing at the same host:port, they are distinct instances and the same seed can be selected twice while a different seed is left out of the probe set. Adding equals/hashCode on (target, port) would make the dedup and the top-up loop deterministic.
| "1 100 8443 near-b.example.com", | ||
| "1 50 19530 near-a.example.com.", | ||
| ". 0 0 unusable.example.com.", // RFC 2782 root target: service unavailable | ||
| "not numbers 8443 bad.example.com", |
There was a problem hiding this comment.
sdk-core/src/test/java/io/milvus/v2/client/globalcluster/SrvDiscoveryTest.java line:78
Low ---- The comment says this record exercises the RFC 2782 root-target skip, but the period is in the PRIORITY field (. 0 0 unusable.example.com.), so Integer.parseInt(".") throws and the record is skipped as malformed. The actual root-target path — a target field of . like "1 0 0 ." — is never exercised; parseSrvRecords' target.endsWith(".") branch is left uncovered.
| * server-side rejections. The global cluster is identified by the raw hostname, passed | ||
| * verbatim via the {@code ?endpoint=} query parameter. | ||
| */ | ||
| static GlobalTopology probeSeed(SrvTarget target, String hostname, String token) |
There was a problem hiding this comment.
sdk-core/src/main/java/io/milvus/v2/client/globalcluster/GlobalClusterUtils.java line:384
Medium ---- The documented fail-fast auth semantics only hold when the server returns HTTP 200 with a JSON code != 0. doHttpGet throws IOException for any non-200 status, so an HTTP 401/403 auth rejection from the ha-manager is classified as a transient seed error and retried 3x with backoff, then surfaced as a generic RuntimeException — never as the GlobalClusterApiException the PR description promises for server-side auth errors. If the topology API reports bad/expired credentials via HTTP 401/403 rather than a 200 error body, the initial connect on an invalid token stalls through every retry and gives the caller an unclear failure. Consider mapping 401/403 (or auth-specific statuses) to GlobalClusterApiException in probeSeed/doHttpGet so auth failures fail fast and expose the code as intended.
| } | ||
| logger.info("Global topology: discovered version {} (cached {}), triggering reconnection", | ||
| newer.getVersion(), cachedVersion); | ||
| onTopologyChange.accept(newer); |
There was a problem hiding this comment.
sdk-core/src/main/java/io/milvus/v2/client/globalcluster/TopologyRefresher.java line:127
Medium ---- The boolean returned by onTopologyChange (the CAS outcome) is discarded here because it is typed as a void Consumer, so when the CAS rejects the candidate as stale the refresh still logs "triggering reconnection" even though nothing was installed. Given the strict no-rollback design this PR introduces, an update path that reports success while being ignored makes the failure of a primary swap harder to observe. Consider accepting the boolean (e.g., log "topology candidate rejected as stale") or documenting that the return value is intentionally ignored.
Why
Global cluster connections previously fetched topology by hitting the global endpoint
directly (
https://<endpoint>/global-cluster/topology). This PR switches the Java SDK tothe same SRV-based discovery as pymilvus (milvus-io/pymilvus#3753): discover ha-manager
seed servers through the
_grpc._tcp.<hostname>SRV record set, probe them concurrently,and use the topology with the highest version — with strict no-rollback semantics for
every refresh path.
What changed
GlobalClusterUtils): resolve_grpc._tcp.<endpoint hostname>to theha-manager seed list via the JDK's built-in JNDI DNS provider — no new dependency.
Endpoints without SRV records raise instead of falling back to the legacy
direct-endpoint path. The endpoint hostname is passed verbatim via
?endpoint=; theserver derives the global cluster id from its first DNS label, so the SDK does no gcid
parsing (
buildTopologyUrlis removed).is returned — one weighted pick per SRV priority group, uniformly at random when all
weights are zero. The first fetch (no cached version) returns the first seed's answer
immediately; the remaining seeds keep being polled in a background watcher thread and
trigger the topology replacement flow if a higher version arrives. Refreshes with a
cached version poll all seeds and take the highest, so partial replication cannot
regress the cache.
GlobalStub):onTopologyChangeis now acompare-and-set that only installs a topology whose version strictly advances the
shared one — every update path (initial-fetch watcher, periodic refresh,
UNAVAILABLE-triggered refresh) funnels through it. The update runs inside the stub
lock, so topology advancement and primary-client swap stay atomic and roll back
together on failure. Updates arriving after
close()are ignored.TopologyRefresher): no private version snapshot anymore; therefresher reads the current version from the stub's shared topology via a
Supplierand hands candidates back through the CAS, so no path can regress the version.
backoff (3 attempts); deterministic rejections (no SRV records, server-side auth errors,
surfaced via the new
GlobalClusterApiException) fail fast without retry.Notes
endpoints without SRV records will raise instead of falling back to the legacy path.
absorbed by concurrent probing, topology fetched, and the full
create/load/insert/flush/search/drop flow passed through the GlobalStub path.
Tests
SrvDiscoveryTest(white-box, same package): SRV record parsing/sorting/RFC 2782root-target skip, weighted priority-spanning candidate selection, hostname extraction,
verbatim
?endpoint=URL, cached-version gating (no regression), first-answer-wins +background higher-version replacement, first-fetch-probes-2 vs refresh-probes-all,
API-error fail-fast, transient seed/DNS retry then success, retry exhaustion,
missing-SRV fail-fast.
GlobalStubTest(stale-version rejection, post-close rejection),TopologyRefresherTest/GlobalClusterTestfor the new refresher signature.mvn -pl sdk-core test: 2204 passed.