Copilot commented on code in PR #1114:
URL:
https://github.com/apache/skywalking-banyandb/pull/1114#discussion_r3193944163
##########
banyand/trace/block_writer.go:
##########
@@ -152,7 +152,6 @@ type blockWriter struct {
minTimestamp int64
totalMinTimestamp int64
totalMaxTimestamp int64
- minTimestampLast int64
maxTimestamp int64
totalBlocksCount uint64
Review Comment:
`minTimestamp`/`maxTimestamp` are updated/reset in this writer but never
read (primaryBlockMetadata only stores traceID/offset/size, and Flush uses
totalMin/totalMax). Now that `minTimestampLast` is removed, consider removing
these unused fields and the associated update/reset logic to reduce dead state
and make the writer’s invariants clearer.
##########
banyand/trace/block_writer_test.go:
##########
@@ -165,6 +165,56 @@ func TestMultiplePrimaryBlockFlushWithPartIter(t
*testing.T) {
}
}
+// TestBlockWriter_OutOfOrderTimestampsSameTraceID exercises the scenario from
+// https://github.com/apache/skywalking/issues/13860: SkyWalking OAP forwards
+// segments produced by multiple agents whose wall clocks drift relative to
+// each other, so a later batch for the same traceID can carry timestamps
+// earlier than an earlier batch. Spans of one trace have no meaningful order
+// here — the writer must accept them without panicking.
+func TestBlockWriter_OutOfOrderTimestampsSameTraceID(t *testing.T) {
+ const tid = "6359c73a002c425785500f958cdc4007.661.17779941290647127"
+
+ mp := &memPart{}
+ mp.reset()
+ bw := generateBlockWriter()
+ bw.MustInitForMemPart(mp, 1)
+
+ tagsFor := func(i int) []*tagValue {
+ return []*tagValue{
+ {tag: "service", valueType: pbv1.ValueTypeStr, value:
[]byte(fmt.Sprintf("svc-%d", i))},
+ }
+ }
+ mustWrite := func(timestamps []int64) {
+ spans := make([][]byte, len(timestamps))
+ spanIDs := make([]string, len(timestamps))
+ tagSets := make([][]*tagValue, len(timestamps))
+ for i, ts := range timestamps {
+ spans[i] = []byte(fmt.Sprintf("span-%d", ts))
+ spanIDs[i] = fmt.Sprintf("span-%d", ts)
+ tagSets[i] = tagsFor(i)
+ }
+ bw.MustWriteTrace(tid, spans, tagSets, timestamps, spanIDs)
+ }
+
+ // First batch: minTS=1_777_994_129_833_000_000.
+ mustWrite([]int64{1_777_994_129_833_000_000, 1_777_994_129_900_000_000})
+ // Second batch for the same tid carries an earlier minTS — this used to
+ // trip block_writer's `tm.min < bw.minTimestampLast` panic and discard
+ // the batch on production traffic.
Review Comment:
The comment references `bw.minTimestampLast`, but that field was removed in
this change. Consider rewording to describe the removed invariant/check (e.g.,
“per-traceID timestamp monotonicity check”) rather than a non-existent internal
field name, to avoid confusion for future maintainers.
--
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]
