You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by "Anon2Tokyo (via GitHub)" <gi...@apache.org> on 2026/08/27 14:20:04 UTC
[PR] fix: address follow-up issues from #1017 [dubbo-go-pixiu]
Anon2Tokyo opened a new pull request, #1023:
URL: https://github.com/apache/dubbo-go-pixiu/pull/1023
**What this PR does**:
This PR addresses a few follow-up issues found after #1017 was merged:
- Ignore stale endpoint removal events when the current cluster snapshot already contains the endpoint.
- Preserve OpenTelemetry Baggage in Dubbo attachments when there is no span.
- Fix incorrect Triple via-Pixiu data in `tools/benchmark/README_CN.md`.
- Add a check to keep the English and Chinese Triple benchmark tables in sync.
**Which issue(s) this PR fixes**:
Follow-up fixes for #1017.
**Special notes for your reviewer**:
This change contains three independent follow-ups to #1017.
1. Endpoint removal event ordering
`grpcConnectionManager` keeps tombstones for removed endpoints so an in-flight dial cannot publish an old connection after the endpoint has been removed. Tombstones are bounded, and eviction also removes the endpoint event version.
The problematic sequence is:
```text
endpoint A is removed
-> A's tombstone is evicted after endpoint churn
-> the current cluster snapshot contains A again
-> an older remove callback arrives
-> A is marked removed again because its old version watermark was evicted
```
A later request can see A in the cluster snapshot but still fail in `pinEndpoint` with `grpc endpoint was removed`.
The removal path now checks the current cluster snapshot before recording a `present=false` event. If the endpoint is currently present, the callback is stale and is ignored. This keeps tombstone state bounded instead of retaining every endpoint version indefinitely.
Covered by `TestGRPCConnectionManagerIgnoresStaleRemovalAfterTombstoneEviction`.
2. Baggage propagation in Dubbo attachments
`withAttachments` previously returned the original context when tracing was disabled, there were no business attachments, and the context had no valid `SpanContext`.
That is not enough to determine whether a propagator has work to do. A propagator can inject OpenTelemetry Baggage without a span. For example, a context containing only `tenant=blue` previously reached the Dubbo invocation without a `baggage` attachment.
The fast path now injects into a temporary carrier first. It returns the original context only when the carrier remains empty. The carrier allocates its backing map lazily, so no attachment map is created when there is nothing to propagate.
Covered by `TestWithAttachmentsPropagatesBaggageWithoutSpanWhenTracingDisabled`.
3. Triple benchmark documentation
The Chinese README labeled Triple direct values as `via Pixiu` for `GetUser` and `GetUsers`. The values now match the Triple via-Pixiu rows in the English README.
`TestTripleBenchmarkReadmesAgree` compares the documented direct and via-Pixiu Triple rows between the English and Chinese README files to catch future drift.
Tests:
```text
go test ./pkg/filter/http/grpcproxy -run '^TestGRPCConnectionManager(IgnoresStaleRemovalAfterTombstoneEviction|RejectsEvictedRemovedEndpointFromSnapshot|BoundsEndpointTombstones)$' -count=1 -timeout 30s
go test ./pkg/client/dubbo -run '^TestWithAttachmentsPropagates(BaggageWithoutSpanWhenTracingDisabled|ExternalSpanWhenTracingDisabled)$' -count=1 -timeout 30s
cd tools/benchmark && go test . -run '^TestTripleBenchmarkReadmesAgree$' -count=1 -timeout 30s
```
**Does this PR introduce a user-facing change?**:
```release-note
Fixes stale endpoint removal handling and preserves OpenTelemetry Baggage in Dubbo proxy requests.
```
--
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@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix: address follow-up issues from #1017 [dubbo-go-pixiu]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #1023:
URL: https://github.com/apache/dubbo-go-pixiu/pull/1023#issuecomment-5440500710
## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=1023) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=1023&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=1023&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go-pixiu&pullRequest=1023&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=1023&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=1023&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=1023)
--
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@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix: address follow-up issues from #1017 [dubbo-go-pixiu]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1023:
URL: https://github.com/apache/dubbo-go-pixiu/pull/1023#issuecomment-5549320187
## [Codecov](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/1023?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `68.00000%` with `8 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 35.49%. Comparing base ([`6a49883`](https://app.codecov.io/gh/apache/dubbo-go-pixiu/commit/6a498838d50d88fe978748e302c25d8d67e9f98a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`d52cdaa`](https://app.codecov.io/gh/apache/dubbo-go-pixiu/commit/d52cdaa9a2be4655e81ea90de36b1228a4a8757b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 3 commits behind head on develop.
| [Files with missing lines](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/1023?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [pkg/client/dubbo/dubbo.go](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/1023?src=pr&el=tree&filepath=pkg%2Fclient%2Fdubbo%2Fdubbo.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL2NsaWVudC9kdWJiby9kdWJiby5nbw==) | 65.00% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/1023?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [pkg/filter/http/grpcproxy/grpc.go](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/1023?src=pr&el=tree&filepath=pkg%2Ffilter%2Fhttp%2Fgrpcproxy%2Fgrpc.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL2ZpbHRlci9odHRwL2dycGNwcm94eS9ncnBjLmdv) | 66.66% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/1023?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## develop #1023 +/- ##
===========================================
+ Coverage 35.31% 35.49% +0.17%
===========================================
Files 292 292
Lines 25181 25201 +20
===========================================
+ Hits 8893 8945 +52
+ Misses 15418 15385 -33
- Partials 870 871 +1
```
| [Flag](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/1023/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [unittests](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/1023/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `35.49% <68.00%> (+0.17%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/1023?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix: address follow-up issues from #1017 [dubbo-go-pixiu]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #1023:
URL: https://github.com/apache/dubbo-go-pixiu/pull/1023#issuecomment-5452294049
## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=1023) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=1023&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=1023&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go-pixiu&pullRequest=1023&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=1023&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=1023&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=1023)
--
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@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix: address follow-up issues from #1017 [dubbo-go-pixiu]
Posted by "AlexStocks (via GitHub)" <gi...@apache.org>.
AlexStocks commented on code in PR #1023:
URL: https://github.com/apache/dubbo-go-pixiu/pull/1023#discussion_r3883697499
##########
pkg/filter/http/grpcproxy/connection_manager_test.go:
##########
@@ -253,6 +257,108 @@ func TestGRPCConnectionManagerRejectsEvictedRemovedEndpointFromSnapshot(t *testi
require.Zero(t, dialCalls.Load())
}
+func TestFilterFactoryIgnoresStaleRemovalAfterTombstoneEviction(t *testing.T) {
+ const clusterName = "stale-removal-cluster"
+
+ endpoint := startTestGRPCServer(t)
+ primary := &model.Endpoint{
+ ID: "primary",
+ Name: "primary",
+ Address: model.SocketAddress{Address: "127.0.0.1"},
+ }
+ require.NoError(t, parseEndpointPort(primary, endpoint))
+
+ clusterManager := server.CreateDefaultClusterManager(&model.Bootstrap{
+ StaticResources: model.StaticResources{
+ Clusters: []*model.ClusterConfig{
+ {
+ Name: clusterName,
+ LbStr: model.LoadBalancerRoundRobin,
+ Endpoints: []*model.Endpoint{primary},
+ },
+ },
+ },
+ })
+ factory := (&Plugin{}).newFilterFactory(clusterManager)
Review Comment:
[P1] 这条用例仍不能证明 snapshot guard 和公开 factory wiring 生效
这里直接调用私有 `newFilterFactory(clusterManager)`，没有经过 `Plugin.CreateFilterFactory -> server.GetClusterManager` 的生产入口；即使公开入口未正确取得或安装 ClusterManager，本测试仍会通过。当前事件顺序也没有真正淘汰 primary 的 version：primary 在 tombstone churn 前已经通过 `UpdateCluster` 重新加入并获得更新的 `present=true` version，后续删除的只是 `churn-*` 地址，所以即使移除 `UpdateEndpointState` 新增的 snapshot guard，延迟的旧 removal 仍会被现有 `eventVersion <= endpointEventVers[key]` 判断丢弃。请从公开 `CreateFilterFactory` 建立 factory，并先让 primary 的 removal/version 进入 tombstone、经 churn 被淘汰，再 re-add primary 并释放旧 callback；同时用删除 snapshot guard 的 mutant 证明该用例会失败。
--
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@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix: address follow-up issues from #1017 [dubbo-go-pixiu]
Posted by "Anon2Tokyo (via GitHub)" <gi...@apache.org>.
Anon2Tokyo commented on PR #1023:
URL: https://github.com/apache/dubbo-go-pixiu/pull/1023#issuecomment-5440580359
@AlexStocks 哈喽你好，这是PR是为了解决 之前 https://github.com/apache/dubbo-go-pixiu/pull/1017 遗漏的三个问题：
1.endpoint re-add 后旧 remove 事件乱序导致健康实例被拒绝；
2.Dubbo tracing fast path 丢失仅包含 Baggage 的传播；
3.README_CN 的 Triple benchmark 数据/标签与英文不一致。
麻烦你有空的话CR一下，感谢你的时间
--
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@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix: address follow-up issues from #1017 [dubbo-go-pixiu]
Posted by "AlexStocks (via GitHub)" <gi...@apache.org>.
AlexStocks commented on code in PR #1023:
URL: https://github.com/apache/dubbo-go-pixiu/pull/1023#discussion_r3878271065
##########
pkg/client/dubbo/call_test.go:
##########
@@ -250,6 +251,24 @@ func TestWithAttachmentsPropagatesExternalSpanWhenTracingDisabled(t *testing.T)
require.Equal(t, "00-01000000000000000000000000000000-0200000000000000-01", attachments["traceparent"])
}
+func TestWithAttachmentsPropagatesBaggageWithoutSpanWhenTracingDisabled(t *testing.T) {
+ restorePropagator(t, propagation.NewCompositeTextMapPropagator(propagation.TraceContext{}, propagation.Baggage{}))
+ previousTracingEnabled := tracingEnabled.Load()
+ t.Cleanup(func() { SetTracingEnabled(previousTracingEnabled) })
+ SetTracingEnabled(false)
+
+ member, err := baggage.NewMember("tenant", "blue")
+ require.NoError(t, err)
+ bag, err := baggage.New(member)
+ require.NoError(t, err)
+ ctx := baggage.ContextWithBaggage(context.Background(), bag)
+ ctx = withAttachments(ctx, nil)
Review Comment:
[P1] 回归测试需要经过 `Client.Call` 的真实生产链
这里直接调用私有 `withAttachments`，只证明 helper 能把 baggage 写进 context；即使后续 `Client.Call` 不再调用它、在 `GenericService.Invoke` 前替换 context，或 HTTP outbound 路径没有传入入口 context，本测试仍会通过，因此不能证明 release note 声称的 Dubbo proxy 请求已保留 baggage。请改为调用 `Client.Call`，通过缓存的 `GenericService.Invoke` 回调断言最终 `AttachmentKey` 中的 `baggage=tenant=blue`，并用 Base 或撤销 fast-path 修复的 mutant 证明旧错误会失败。
--
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@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix: address follow-up issues from #1017 [dubbo-go-pixiu]
Posted by "AlexStocks (via GitHub)" <gi...@apache.org>.
AlexStocks commented on code in PR #1023:
URL: https://github.com/apache/dubbo-go-pixiu/pull/1023#discussion_r3878274306
##########
pkg/filter/http/grpcproxy/connection_manager_test.go:
##########
@@ -253,6 +253,50 @@ func TestGRPCConnectionManagerRejectsEvictedRemovedEndpointFromSnapshot(t *testi
require.Zero(t, dialCalls.Load())
}
+func TestGRPCConnectionManagerIgnoresStaleRemovalAfterTombstoneEviction(t *testing.T) {
+ endpoint := startTestGRPCServer(t)
+ current := make(map[string]bool)
+ var currentMu sync.Mutex
+ var dialCalls atomic.Int32
+ manager := testConnectionManager(t, &dialCalls)
+ manager.endpointPresent = func(_, address string) bool {
Review Comment:
[P1] 用真实 ClusterManager wiring 验证 stale-removal 修复
这里直接替换 `grpcConnectionManager.endpointPresent` 并调用内部 `UpdateEndpointState`，绕过了修复真正依赖的 `Plugin.CreateFilterFactory -> ClusterManager.AddEndpointStateHandler -> runtime snapshot` 生产链。即使 factory 没安装 snapshot 回调、handler 没注册或真实 endpoint 发布顺序仍有问题，本测试也会通过。请用真实 ClusterManager mutation/handler wiring 重现 remove、tombstone eviction、re-add、delayed removal，再从 gRPC proxy 使用的 manager 发起连接或请求；同时用 Base 或撤销 snapshot guard 的 mutant 证明旧错误会失败。
--
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@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix: address follow-up issues from #1017 [dubbo-go-pixiu]
Posted by "Similarityoung (via GitHub)" <gi...@apache.org>.
Similarityoung commented on code in PR #1023:
URL: https://github.com/apache/dubbo-go-pixiu/pull/1023#discussion_r4053026681
##########
pkg/filter/http/grpcproxy/connection_manager.go:
##########
@@ -373,6 +373,14 @@ func (m *grpcConnectionManager) RemoveEndpoint(clusterName, endpoint string) {
// a connection created after the endpoint was re-added.
func (m *grpcConnectionManager) UpdateEndpointState(clusterName, endpoint string, present bool, eventVersion uint64) {
key := grpcConnectionKey(clusterName, endpoint)
+ // Lifecycle callbacks are delivered outside the cluster manager lock and can
+ // therefore arrive out of order. Once a bounded tombstone has been evicted,
+ // its version watermark is no longer available to reject an old removal.
+ // The cluster snapshot is authoritative in that case: do not let a delayed
+ // removal turn an endpoint that is currently present back into a tombstone.
+ if !present && m.isEndpointPresent(key, endpoint) {
Review Comment:
isEndpointPresent() 在 endpointPresent == nil 时会返回 true，所以这里会直接跳过所有 present=false 的事件。
建议只在 endpointPresent != nil 时做这个判断。
另外，当前 tombstone 测试也可能因此变成空跑，最好补一个断言确认 removal state 确实被记录。
--
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@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org