You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "mrproliu (via GitHub)" <gi...@apache.org> on 2026/05/12 14:04:55 UTC
[PR] fix(property): prevent gossip propagation from getting stuck after replica scale-up [skywalking-banyandb]
mrproliu opened a new pull request, #1127:
URL: https://github.com/apache/skywalking-banyandb/pull/1127
### Fix apache/skywalking#13852
- [x] Add a unit test to verify that the fix works.
- [x] Explain briefly why the bug exists and how to fix it.
#### Why the bug exists
After replica scale-up, the very first round of property repair could deadlock the gossip scheduler. Two latent issues compound:
1. **`size=1` channel with drop-on-full in `groupWithShardPropagation`** — when a handler was slow (e.g. a long-running per-property `repair`), subsequent same-round / TTL-takeover requests hit the `default` branch of the `select` and were silently dropped. Once dropped, the round was never retried — the scheduler appeared healthy but no progress was made.
2. **`groupNotify` cap=10 with drop-on-full** — under bursty load, signals could be coalesced away while pending entries existed, leaving them stranded because the worker consumed exactly one pending per wake-up.
3. **No per-property timeout on `syncShard.repair`** — a single slow property lookup could consume the entire `perNodeSyncTimeout` (1h) budget, stalling the whole repair stream.
#### How it's fixed
- **`banyand/property/gossip/server.go`**: replace the per-shard `channel chan *handlingRequest` with a `pending *handlingRequest` pointer using **latest-wins coalesce** semantics. Overwriting a non-nil `pending` increments a new `totalCoalesced` metric and logs at Info (instead of dropping + logging Error). The `processPropagation` worker now **drains all ready pending entries per wake-up**, with an inner `CloseNotify` re-check so `GracefulStop` stays responsive even with backlog.
- **`banyand/property/db/repair_gossip.go`**: wrap each `syncShard.repair(...)` call (client `processDifferTreeSummary` and server `processPropertySync` paths) with `context.WithTimeout(ctx, repairPerPropertyTimeout)` (10s). A single slow property no longer monopolizes the per-node sync budget; the existing per-entry error handling cleanly skips a timed-out property and continues the loop/stream.
- New metric: `total_coalesced{group}`.
#### Tests
- `banyand/property/gossip/service_test.go`: 5 new ginkgo cases covering latest-wins reach-Rev, TTL-expired takeover (empty + non-nil pending), drain-all per wake-up under saturated `groupNotify`, and `CloseNotify` honored during drain.
- `banyand/property/db/repair_gossip_test.go`: 1 new case verifying a single property repair timeout does not abort the round.
- All existing tests pass under `-race`.
---
- [x] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Fixes apache/skywalking#13852.
- [x] Update the [`CHANGES` log](https://github.com/apache/skywalking-banyandb/blob/main/CHANGES.md).
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(property): prevent gossip propagation from getting stuck after replica scale-up [skywalking-banyandb]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1127:
URL: https://github.com/apache/skywalking-banyandb/pull/1127#discussion_r3227096687
##########
banyand/property/db/repair_gossip.go:
##########
@@ -307,13 +357,15 @@ func (r *repairGossipClient) Rev(ctx context.Context, tracer gossip.Trace, nextN
syncSpan := tracer.CreateSpan(startSyncSpan, "repair property")
syncSpan.Tag(gossip.TraceTagOperateType, gossip.TraceTagOperateRepairProperty)
syncSpan.Tag(gossip.TraceTagPropertyID, string(sync.Property.Id))
- updated, newer, err := syncShard.repair(ctx, sync.Property.Id, sync.Property.Property, sync.Property.DeleteTime)
+ updated, newer, err := r.executeRepairWithBudget(ctx, syncShard, sync.Property.Id, sync.Property.Property, sync.Property.DeleteTime, request.Group)
syncSpan.Tag("updated", fmt.Sprintf("%t", updated))
syncSpan.Tag("has_newer", fmt.Sprintf("%t", newer != nil))
if err != nil {
Review Comment:
In the PropertySync handling block, `syncSpan.End()` isn’t guaranteed to run. If `updated == false` and either `newer == nil` or `sync.From == ...MISSING`, the code falls through without ending the span, which can leak spans and distort tracing. Consider deferring `syncSpan.End()` immediately after creation (or ensuring it’s called on every exit path of this case).
##########
banyand/property/gossip/server.go:
##########
@@ -328,23 +336,31 @@ func (q *protocolHandler) addToProcess(request *propertyv1.PropagationRequest, t
// if the latest round is out of ttl, then needs to change to current node to executing
if time.Since(groupShard.latestTime) > q.s.scheduleInterval/2 {
groupShard.originalNodeID = request.Context.OriginNode
- select {
- case groupShard.channel <- handlingRequestData:
- q.notifyNewRequest()
- default:
- q.s.log.Error().Msgf("ready to added propagation into group shard %s(%d) in a new round, but it's full", request.Group, request.ShardId)
+ if groupShard.pending != nil {
+ q.s.serverMetrics.totalCoalesced.Inc(1, request.Group)
+ q.s.log.Info().
+ Str("group", request.Group).
+ Uint32("shardID", request.ShardId).
+ Str("originNode", request.Context.OriginNode).
+ Msg("propagation request coalesced into pending (TTL takeover)")
}
+ groupShard.pending = handlingRequestData
+ q.notifyNewRequest()
return true
Review Comment:
`groupShard.latestTime` is only initialized on first creation and never updated when new requests are accepted into `pending`. This makes the TTL check `time.Since(groupShard.latestTime) > q.s.scheduleInterval/2` use a stale timestamp, so a different originator can be accepted immediately after a recent request (because the TTL condition stays true forever after the first expiry). Update `latestTime` whenever you enqueue/overwrite `pending` so the TTL window is based on the most recent accepted round/activity.
##########
test/property_repair/shared_utils.go:
##########
@@ -334,6 +354,106 @@ func parseRepairSuccessCount(content string) int64 {
return totalCount
}
+// parseRepairFailureCount parses the repair_failure_count from prometheus metrics text.
+func parseRepairFailureCount(content string) int64 {
+ // Look for metric lines like: banyandb_property_scheduler_property_repair_failure_count{group="perf-test-group",shard="0"} 5
+ re := regexp.MustCompile(`banyandb_property_scheduler_property_repair_failure_count\{[^}]+\}\s+(\d+(?:\.\d+)?)`)
+ matches := re.FindAllStringSubmatch(content, -1)
+
+ var totalCount int64
+ for _, match := range matches {
+ if len(match) >= 2 {
Review Comment:
The Prometheus value regexes (e.g., `\d+(?:\.\d+)?`) won’t match valid exposition formats like scientific notation (e.g., `1.23e+06`) or `+Inf` (histograms). This can cause these parsers to silently return 0 and make the integration/perf checks flaky. Consider using a more complete Prometheus float pattern (optionally including exponent and `Inf`/`NaN`) and reusing it across all `parse*` helpers.
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(property): prevent gossip propagation from getting stuck after replica scale-up [skywalking-banyandb]
Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily merged PR #1127:
URL: https://github.com/apache/skywalking-banyandb/pull/1127
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(property): prevent gossip propagation from getting stuck after replica scale-up [skywalking-banyandb]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1127:
URL: https://github.com/apache/skywalking-banyandb/pull/1127#issuecomment-4438823978
## [Codecov](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1127?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 34.69%. Comparing base ([`3530dd9`](https://app.codecov.io/gh/apache/skywalking-banyandb/commit/3530dd9b5cefaa4fef60eec91ecbfbdfcaf243c5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`6c10a43`](https://app.codecov.io/gh/apache/skywalking-banyandb/commit/6c10a43527f45b3c382ab01d334c34fbc1d74f84?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 251 commits behind head on main.
> :exclamation: There is a different number of reports uploaded between BASE (3530dd9) and HEAD (6c10a43). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (3530dd9) | HEAD (6c10a43) |
>|------|------|------|
>||1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #1127 +/- ##
===========================================
- Coverage 45.97% 34.69% -11.29%
===========================================
Files 328 149 -179
Lines 55505 15231 -40274
===========================================
- Hits 25520 5284 -20236
+ Misses 27909 9437 -18472
+ Partials 2076 510 -1566
```
| [Flag](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1127/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [pkg](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1127/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.69% <ø> (?)` | |
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 Sentry](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1127?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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(property): prevent gossip propagation from getting stuck after replica scale-up [skywalking-banyandb]
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu commented on code in PR #1127:
URL: https://github.com/apache/skywalking-banyandb/pull/1127#discussion_r3231375763
##########
banyand/property/gossip/server.go:
##########
@@ -328,23 +336,31 @@ func (q *protocolHandler) addToProcess(request *propertyv1.PropagationRequest, t
// if the latest round is out of ttl, then needs to change to current node to executing
if time.Since(groupShard.latestTime) > q.s.scheduleInterval/2 {
groupShard.originalNodeID = request.Context.OriginNode
- select {
- case groupShard.channel <- handlingRequestData:
- q.notifyNewRequest()
- default:
- q.s.log.Error().Msgf("ready to added propagation into group shard %s(%d) in a new round, but it's full", request.Group, request.ShardId)
+ if groupShard.pending != nil {
+ q.s.serverMetrics.totalCoalesced.Inc(1, request.Group)
+ q.s.log.Info().
+ Str("group", request.Group).
+ Uint32("shardID", request.ShardId).
+ Str("originNode", request.Context.OriginNode).
+ Msg("propagation request coalesced into pending (TTL takeover)")
}
+ groupShard.pending = handlingRequestData
+ q.notifyNewRequest()
return true
Review Comment:
Reviewed and intentionally not refreshing latestTime: if the take-over node also stalls, refreshing would force the next originator to wait another scheduleInterval/2 (~1h) before it can re-take
--
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@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org