Skip to content

fix(dubbo): keep provider identity through Dubbo bookkeeping and count inflight in a consumer filter (#8132) - #8183

Open
konlue wants to merge 4 commits into
apache:masterfrom
konlue:fix/dubbo-inflight-wrapper-identity
Open

konlue wants to merge 4 commits into
apache:masterfrom
konlue:fix/dubbo-inflight-wrapper-identity

Conversation

@konlue

@konlue konlue commented Oct 28, 2026

Copy link
Copy Markdown
Contributor

Fixes #8132.

Problem

The least-active gray path returned a counting wrapper from the load balance. Dubbo's cluster keeps whatever the LB returns in its already-tried list, and reselect compares raw directory candidates against that list: candidate.equals(wrapper) runs on the raw invoker's own equality and can never recognize the wrapper, no matter how the wrapper implements its own side. A failed provider was therefore admitted back into the retry candidates even when another provider was available. This reproduces the probe from the issue against the real AbstractClusterInvoker (Dubbo 3.2.14).

Change

  • Restore canonical identity: The gray load balance returns the bare directory invoker again, so every Dubbo bookkeeping list (already-tried, connectivity invalidation) keeps canonical provider identity.
  • Move accounting to a filter: Inflight accounting moves to a consumer-side Dubbo filter (DubboGrayInflightFilter, activated for consumers via SPI). The load balance marks the upstream it selected for the current attempt on a local invocation attachment; the filter runs after the cluster committed to a provider, resolves the upstream by selector and url, counts the attempt exactly once (released on completion or failure), and removes the marker so it never reaches the provider. Being per-attempt and per-provider, the filter also counts reselected attempts correctly — something the wrapper silently missed, since a reselected invoker bypassed it.
  • Remove the wrapper: The InflightInvoker wrapper is removed.
  • Limitation: An upstream that is not addressable by url (group/version-only gray config) is not marked and not counted; the filter cannot re-identify it. This is noted in the load balance javadoc.

Tests

  • GrayLoadBalanceClusterIdentityTest: Drives the real AbstractClusterInvoker.select over the real gray load balance with a deterministic test SPI load balance. With provider A already tried and B available, the reselect must choose B. Red on the old code (expected b:1 but was a:1, the issue's probe), green after the change.
  • Accounting tests: The three exactly-once accounting tests moved to the filter (count while the RPC runs, release on failure, release each invocation on a reused invoker), plus: no marker means no counting, the marker is removed before the call continues, and an unresolvable marker passes through untouched.
  • Identity assertions: The load balance tests now assert the bare invoker identity and the marker attachment.

Verification

shenyu-plugin-apache-dubbo: 62/62
dubbo-common: 321/321
checkstyle: clean

(plus the rest of the reactor)

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the canonical-invoker/filter refactor at cdcc026. Returning the original directory invoker resolves the identity/bookkeeping problem, and the new consumer filter activates through Dubbo SPI. However, two production-class probes reproduce incorrect accounting targets after cluster reselection and with same-address group/version targets (inline).

Validation: all 62 apache-dubbo and 12 dubbo-common tests passed. Probes used the actual Dubbo 3.2.14 AbstractClusterInvoker, ShenYu leastActive implementation, upstream cache and new filter; provider transport/results were controllable mocks. Success, exceptional source completion and source-future cancellation release checks passed. No live Dubbo network/registry failover or full reactor/e2e validation is claimed.

if (Objects.isNull(upstreamUrl)) {
return invoker.invoke(invocation);
}
Upstream upstream = matchUpstream(invocation, String.valueOf(upstreamUrl));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Attribute reselection attempts to the committed provider, not the earlier LB marker

The marker identifies an LB proposal, not necessarily the provider the cluster finally invokes. With two healthy providers A/B, A.inflight=0 and B.inflight=1, and A already in Dubbo's tried list, the real AbstractClusterInvoker.select first lets this LB select/mark A, then reselects B. The reselection calls this LB with only B; the gray upstream lookup still picks A from the full cache, so invokerGrays is empty and the fallback returns B without replacing/clearing the marker. This filter ignores invoker.getUrl() and increments A anyway. I reproduced this with the actual leastActive implementation and Dubbo 3.2.14 cluster: actual=b:1 marker=a:1 aDelta=1 bDelta=0. Thus retry load is attributed to the failed/previous provider while the actual serving provider remains undercounted. Resolve the committed invoker against the selector's upstreams (and use a marker only as a safe eligibility hint), or otherwise ensure reselection updates the accounting target; add a cluster-selection-plus-filter regression rather than only asserting the returned invoker identity.

}
return upstreams.stream()
.filter(upstream -> upstreamUrl.equals(upstream.getUrl()))
.findFirst()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Preserve group/version identity when resolving the accounting upstream

Selecting by URL alone collapses distinct Dubbo gray targets that share an address. The cache intentionally keeps logical identities including protocol/address/group/version, and ApacheDubboGrayLoadBalance filters invokers by group/version, but the new marker stores only upstream.getUrl() and findFirst() always resolves the first address match. With shared:1, group=g, version=1 and shared:1, group=g, version=2 in the same selector, I used the real leastActive balancer to select version 2; the filter increments version 1 instead: actualVersion=2 v1Delta=1 v2Delta=0. This regresses the removed wrapper, which retained the exact chosen Upstream. Use the complete logical identity / actual provider group and version, and add a same-address multi-version regression so weights and outstanding-load decisions remain scoped to the selected target.

@konlue

konlue commented Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the probes — both findings were real and are fixed in the new commit.

  1. Reselection: The marker was treated as the accounting target, but the cluster can reselect after the load balance returns without calling it again, so the marker may name a provider that is not serving the request. The marker is now only an eligibility hint; the filter resolves invoker.getUrl() — address, group and version — against the selector's upstreams and counts that one. An unresolvable committed provider is not counted; it never falls back to the marker.

  2. Same-address group/version: Identity was URL-only, and same-address upstreams with different group/version are distinct UpstreamIdentity values in the cache, so findFirst() was ambiguous by construction. Resolution now uses address + group + version; a blank configured group/version is a wildcard mirroring ApacheDubboGrayLoadBalance's own invoker filtering, and non-blank fields must match exactly. The documented URL-unaddressable limitation is unchanged.

  3. Regressions:

    • reselectCountsTheCommittedProviderNotTheMarkedOne: Drives the real AbstractClusterInvoker twice (A tried → B reselected) and runs the filter with committed B while the marker still names A. Previous code gave aΔ=1, bΔ=0; now aΔ=0, bΔ=1, returning to baseline on completion.
    • countsTheUpstreamMatchingTheCommittedGroupAndVersion: Uses two upstreams on shared:1 with (g,1) / (g,2) and a committed group=g&version=2. Previous code incremented v1; now v2.
    • An unknown-provider case and a blank-group wildcard case are covered too.

Both probes are red on the previous commit and green now; filter suite 9/9, cluster identity 2/2.

Verification:

apache-dubbo: 67/67
shenyu-plugin-dubbo-common: 12/12
shenyu-plugin-base: 321/321
shenyu-common: 626 (4 skipped)

checkstyle clean on the touched modules. No live registry/network failover claim.

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed bd833e1. The original committed-provider and exact group/version reproductions are now green, and the consumer filter activates through Dubbo SPI. One remaining overlapping wildcard/exact identity case still misattributes leastActive counts (follow-up to the existing group/version thread).

Validation: 15 targeted Maven tests passed, including real Dubbo cluster selection; additional probes used the actual Dubbo 3.2.14 cluster, real ShenYu leastActive/cache/filter, and controllable transport results. Source completion, exceptional completion and source-future cancellation released counts. No live registry/provider-network failover or full reactor/e2e validation is claimed.

Follow-up to #8183 (comment):

[P2] Preserve the selected logical upstream when wildcard and exact targets overlap

The two original reproducers now pass: reselection counts the committed B, and two exact same-address versions are separated. However, matching the committed provider against every configured entry and taking the first wildcard match still loses the logical target selected by leastActive. An address-only entry shared:1 and an entry shared:1, group=g, version=2 are both accepted and retained as distinct upstreams. With inflight counts 10 and 0, the real balancer selects the specific version-2 entry/provider, but this filter increments the earlier address-only entry: actualVersion=2 genericDelta=1 selectedSpecificDelta=0. The selected entry stays at zero while requests accumulate, so leastActive continues underestimating its outstanding load. Preserve the complete selected upstream identity when it remains compatible with the committed provider; resolve reselection without collapsing overlapping identities. Add this wildcard-plus-specific regression alongside the exact-version case, or explicitly reject/document overlapping configurations rather than attributing by list order.

This configuration is reachable through the existing selector handle, not just a manually constructed cache list. The Admin selector API persists the handle without forbidding same-address overlap; the concrete ApacheDubboPluginDataHandler parses and retains both gray records, and UpstreamIdentity deliberately distinguishes their group/version fields. A separate probe invoked that actual handler, asserted both cached objects survived and asserted real leastActive selected the specific object before the gray LB/filter call. Admin acceptance was verified from source; no running Admin/DB deployment is claimed.

Source: shenyu-plugin/shenyu-plugin-proxy/shenyu-plugin-rpc/shenyu-plugin-dubbo/shenyu-plugin-apache-dubbo/src/main/java/org/apache/shenyu/plugin/apache/dubbo/proxy/DubboGrayInflightFilter.java:106.

@konlue

konlue commented Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — the overlap case is real, and attributing it by list order was the remaining hole.

The marker now carries the selected logical upstream identity (UpstreamIdentity#hashKey()), not just the address. The filter still derives compatible entries from the committed provider (address equal; a blank configured group/version is a wildcard, mirroring the load balance), but it counts the selected identity first while it remains compatible — so an address-only entry no longer steals an attempt from the selected group=g, version=2 entry. When the cluster reselects a different provider, the filter counts the unique most specific compatible entry and never falls back to list order; equally specific overlaps are left uncounted rather than guessed.

Regressions:

  • countsTheSelectedSpecificUpstreamWhenWildcardAndExactOverlap: [shared:1, shared:1/g/2], selected v2 — previous code gives genericΔ=1, v2Δ=0; now genericΔ=0, v2Δ=1.
  • keepsTheSelectedWildcardUpstreamWhenItIsCompatible: the inverse selection stays on the wildcard entry.
  • reselectPrefersTheMostSpecificCompatibleUpstream: marker v1, committed v2 — previous code counts the wildcard; now v2.
  • doesNotCountAmbiguousPartialOverlaps: group-only + version-only tie is not attributed to either entry.

Verification:

Filter suite: 13/13
cluster identity: 2/2
load balance: 4/4
apache-dubbo: 71/71
shenyu-plugin-dubbo-common: 12/12
shenyu-plugin-base: 321/321
shenyu-common: 626 (4 skipped)

checkstyle clean on the touched modules. No live registry/network failover claim.

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed 4f7b1c5. The previously reported reselection, exact same-address group/version and wildcard-plus-exact group/version scenarios are now fixed. The new compatibility check also preserves an intentionally selected wildcard, and the unique-most-specific fallback avoids ambiguous list-order attribution.

One remaining P2 is caused by using the compatibility hash-ring key as a complete logical identity: address-only records with different registry protocols still collide and the filter counts the first object instead of the actual leastActive choice (inline). This is a distinct identity dimension from the now-fixed group/version case.

Validation: 28 focused Maven tests passed, including Checkstyle validation. Old reproductions passed against actual handler/cache/leastActive, real Dubbo 3.2.14 cluster selection and the consumer filter. A new production-handler/real-LB/filter probe failed for the multi-registry identity case; compiling the actual a87a361 LB/wrapper sources into the same isolated fixture made that case pass. Source success, exceptional completion and source-future cancellation released inflight counts, and consumer SPI activation passed. These probes use controllable provider invokers/results; no live ZooKeeper/Nacos/provider network or deployed gateway E2E run is claimed. Corretto 21 compiled the project target 17; RAT was skipped.

return selected;
}
return new InflightInvoker<>(selected, upstream);
invocation.setObjectAttachment(Constants.DUBBO_INFLIGHT_UPSTREAM_IDENTITY, upstream.identity().hashKey());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Use the full logical identity rather than the legacy hash-ring key

UpstreamIdentity.hashKey() is not a unique serialization of UpstreamIdentity: when group and version are absent, it intentionally returns only url (UpstreamIdentity:46-48), although equals() also distinguishes protocol. Consequently the new marker can still collapse two supported logical gray entries. In DubboUpstream this protocol is the registry protocol, consumed by RegistryConfig.setProtocol, rather than the RPC invoker URL scheme.

I reproduced this using the actual selector JSON handler with two enabled gray records for shared:1, no group/version, and different registry configurations: zookeeper / 127.0.0.1:2181 and nacos / 127.0.0.1:8848. The real upstream cache retains two unequal identities, and real leastActive with inflight 10/0 selects the Nacos object. Both markers nevertheless equal shared:1, so the filter's first selectedKey.equals(...hashKey()) match increments the earlier ZooKeeper entry: selectedNacosObject=true; zookeeperDelta=1; selectedNacosDelta=0. The selected logical entry remains idle while calls accumulate. The same fixture using the actual master a87a361 load-balancer/wrapper source increments Nacos instead (0/1), confirming a regression introduced by the refactor.

Use a local marker that preserves the full protocol/address/group/version identity, and match that exact identity while it remains compatible with the committed provider. Keep the legacy hash-ring key unchanged for compatibility. Add this address-only multi-registry regression; do not compare the registry protocol directly to the RPC URL protocol as a substitute.

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6e9ef33.

The previous protocol-collision finding is fixed: the invocation marker preserves protocol, address, group and version separately from the legacy hash-ring key, and the consumer filter attributes the committed invocation to that complete identity. The new real cluster-selection regression distinguishes two address-only upstreams with different registry protocols and verifies paired inflight release.

The earlier change request is resolved.

Targeted test report: 20 cases, 0 failures, 0 errors, 0 skipped.

Validation used ./mvnw, Amazon Corretto 21 and project target Java 17. Targeted Checkstyle, separately executed Apache RAT and git diff --check pass. No full-repository or Docker E2E pass is claimed; required CI must succeed before merge.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Dubbo inflight invoker wrapper breaks provider identity during retry reselection

2 participants