wu-sheng opened a new issue, #13963:
URL: https://github.com/apache/skywalking/issues/13963
> Code paths in this issue are in `apache/skywalking-banyandb`. Umbrella:
#13634.
Found while reviewing apache/skywalking-banyandb#1234. Companion to the
split-block contract issue.
## Problem
At `PIPELINE_EVENT_MERGE` the sampler is evaluated on whatever subset of
parts the LSM compaction policy happened to select. `merge_grace` bounds
**freshness**, not **completeness**:
```go
// banyand/trace/merger.go:368-375
graceNs := lookupMergeGrace(tst.group)
if graceNs <= 0 { graceNs = int64(tst.option.mergeGraceDefault) }
if isMergeHot(parts, graceNs, time.Now().UnixNano()) {
return nil // whole merge runs unfiltered
}
```
`isMergeHot` (`merger.go:771-777`) only asks whether any part in the merge
is younger than the grace window. Part selection is
`mergePolicy.getPartsToMerge` (`banyand/trace/merger_policy.go:53-100`), a
size-tiered search for lowest write amplification with no notion of trace
membership.
A trace whose segments straddled a flush boundary sits in parts P1/P2/P3,
all long past `merge_grace`. The policy picks {P2, P3}. The sampler sees only
those rows, finds no error and no slow envelope, and drops them. The error
segment in P1 survives alone — a truncated trace. Reverse the selection order
and the error evidence itself is destroyed, after which the later pass drops
the remainder.
Only the finalizer selects **all** cooled parts
(`banyand/trace/finalizer.go:85-108`, a full-shard rewrite), so only
`PIPELINE_EVENT_FINALIZE` evaluates something resembling a whole trace — and it
is **off by default** while MERGE is on:
```go
// banyand/trace/metadata.go:296-311
// PIPELINE_EVENT_MERGE installs no samplers. An empty enabled_events list
defaults ...
// pass. Unlike MERGE, an empty enabled_events list does NOT default
finalize on:
```
Out of the box, a whole-trace retention policy therefore runs only at the
fragment-level pass.
## Design-doc divergence
`docs/design/post-trace-pipeline.md` specifies a **per-trace** maturity gate
— lines 325, 363, 568, 581:
> The filter therefore evaluates a trace only once its latest span timestamp
is older than `now − merge_grace` … traces newer than that frontier are passed
through the merge unchanged.
The implementation is a **per-merge, part-level** gate that skips filtering
for the entire merge when any part is fresh. That is stricter, so the "never
drop a still-growing trace" property does hold — but the mechanism the document
describes does not exist, and §7.1 point 6 then draws the wrong conclusion from
it:
> Because a trace is dropped only after `merge_grace` has elapsed since its
last span … the drop is final rather than premature.
"Nothing more is arriving" is not "everything already written is visible in
this merge".
## Suggested direction
1. Document plainly that a MERGE-event verdict is fragment-local, and that
whole-trace policies belong at `PIPELINE_EVENT_FINALIZE`.
2. Consider defaulting FINALIZE on (or refusing a MERGE-only configuration
for plugins that declare a whole-trace policy).
3. Correct §7.1 to describe the part-level gate that is actually implemented.
A true fix at merge time needs a trace→part index so the engine can tell
whether a trace's rows are fully contained in the merged set; short of that,
fragment-local is the honest description.
Related: apache/skywalking-banyandb#1234.
--
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]
