hanahmily commented on code in PR #1120:
URL:
https://github.com/apache/skywalking-banyandb/pull/1120#discussion_r3209165529
##########
banyand/internal/storage/segment.go:
##########
@@ -579,19 +579,37 @@ func (sc *segmentController[T, O]) create(start
time.Time) (*segment[T, O], erro
}
}
options := sc.getOptions()
- start = options.SegmentInterval.Unit.Standard(start)
+ // Anchor stdEnd to the aligned start before any bump so end stays on
the
+ // global grid even when start is bumped past a legacy off-grid
neighbor;
+ // subsequent segments then self-heal back to the grid.
+ alignedStart := options.SegmentInterval.Standard(start)
Review Comment:
### Suffix round-trip is lossy on non-UTC nodes when `Num >= 2` -> adjacent
segments overlap by the local UTC offset after restart
The `alignedStart` computed here flows into `format(start)` at line 617
(called from `sc.format`, lines 491-498), which renders the in-memory bucket
start with `tm.Format(dayFormat)` reading year/month/day from `t.Location()`.
After a process restart, `loadSegments` reconstructs `Start` via
`parseSegmentTime` (lines 502-509), which reparses with
`time.ParseInLocation(dayFormat, value, time.Local)`.
With `Num=1`, `IntervalRule.Standard` falls through to `Unit.Standard` which
already returns local-midnight, so the round-trip is a fixed point. With `Num
>= 2`, `Standard` returns a UTC-anchored instant that is **not** a local
midnight in non-UTC zones, and the round-trip drops the offset.
Repro on an LA node with `SegmentInterval = 15d`:
- Bucket start in memory: `2026-04-07 00:00 UTC` = `2026-04-06 17:00 PDT`
- `format` -> `"20260406"` (LA-local date)
- After restart, `parseSegmentTime("20260406")` -> `2026-04-06 00:00 PDT` =
`2026-04-06 07:00 UTC`
- **Reloaded `Start` is 17 h earlier than the original; `End` is recovered
correctly from `meta.EndTime` (RFC3339Nano carries the offset).** Every
adjacent pair of segments now overlaps by exactly the local UTC offset.
Downstream effects of the overlap:
- The bump loop at lines 591-600 assumes `sc.lst` is non-overlapping; that
invariant is gone after restart.
- The reverse-iteration top-guard at lines 574-580 returns the newest
containing segment, so writes whose timestamps fall in the overlap window get
silently routed to the next bucket's segment instead of the one their data
semantically belongs to.
- `selectSegments` (line 418) produces false-positive matches at every
bucket seam.
`TestIntervalRule_Standard_PreservesLocation` only checks that the bucket
instant is timezone-invariant; it doesn't catch this because the bug is in the
suffix encoding, not the alignment math itself.
##########
banyand/internal/storage/storage.go:
##########
@@ -202,6 +202,41 @@ func (ir IntervalRule) NextTime(current time.Time)
time.Time {
panic("invalid interval unit")
}
+// epochUTC is the Unix epoch as a UTC time. Used as the anchor for the
+// grid produced by IntervalRule.Standard.
+var epochUTC = time.Date(1970, 1, 1, 0, 0, 0, 0, time.UTC)
+
+// Standard aligns t down to the nearest Num*Unit boundary. The grid origin
+// is anchored at the Unix epoch in UTC for cross-node consistency, but the
+// returned time is in t.Location() so callers see their own timezone. For
+// Num<=1 it delegates to IntervalUnit.Standard, preserving the existing
+// per-unit local alignment.
+func (ir IntervalRule) Standard(t time.Time) time.Time {
+ if ir.Num <= 1 {
+ return ir.Unit.Standard(t)
+ }
+ var bucketWidth time.Duration
+ switch ir.Unit {
+ case DAY:
+ bucketWidth = time.Duration(ir.Num) * 24 * time.Hour
+ case HOUR:
+ bucketWidth = time.Duration(ir.Num) * time.Hour
+ default:
+ panic("invalid interval unit")
+ }
+ width := bucketWidth.Nanoseconds()
+ nanos := t.Sub(epochUTC).Nanoseconds()
Review Comment:
### `Standard` (absolute ns) and `NextTime` (calendar `AddDate`) disagree by
1 h around DST transitions -> empty-range segments and 1 h write gaps
This line computes bucket boundaries via `t.Sub(epochUTC).Nanoseconds()`
divided by `Num*24*time.Hour` -- a fixed-width nanosecond grid.
`IntervalRule.NextTime` for `DAY` (lines 199-200 in this file) uses
`current.AddDate(0, 0, ir.Num)` -- calendar-day arithmetic that preserves
wall-clock time across DST. The two metrics agree on UTC nodes but disagree by
1 h whenever a bucket spans a DST transition on a non-UTC node.
In `segmentController.create()` (`segment.go:585-586`):
```go
alignedStart := options.SegmentInterval.Standard(start)
stdEnd := options.SegmentInterval.NextTime(alignedStart)
```
`alignedStart` is a UTC-anchored grid boundary presented in `t.Location()`.
`stdEnd` is then `AddDate(15)` against that local presentation, which preserves
wall-clock 16:00 across the DST transition rather than landing exactly on the
next UTC grid boundary.
Repro on an LA node with `SegmentInterval = 15d`, around 2026 spring-forward
(DST starts March 8):
- Bucket `[2026-03-07 00:00 UTC, 2026-03-22 00:00 UTC)`.
- `alignedStart = 2026-03-07 00:00 UTC = 2026-03-06 16:00 PST`.
- `stdEnd = AddDate(15)` against `2026-03-06 16:00 PST` -> `2026-03-21 16:00
PDT` (by 03-21 we're in PDT) = `2026-03-21 23:00 UTC`.
- Real grid boundary: `2026-03-22 00:00 UTC`. **Segment is created 1 h
short.**
Now a write at `2026-03-21 23:30 UTC` (in the 1 h gap):
- Top guard at `segment.go:574-580`: existing segment ends at `23:00 UTC` ->
does not contain probe.
- `alignedStart = Standard(probe) = 2026-03-07 00:00 UTC` (same bucket).
- `stdEnd = 2026-03-21 23:00 UTC` (same DST bug).
- Bump loop: existing segment contains `alignedStart` -> `start = 2026-03-21
23:00 UTC`.
- Falls through to `else` branch at `segment.go:614-616`, `end = stdEnd =
2026-03-21 23:00 UTC`.
- **`start == end` -- empty-range segment is created and persisted via
`sc.load(start, end, ...)` at `segment.go:639`.**
This fires every spring/fall DST transition on non-UTC nodes with `Num >=
2`, deterministically. The cold-tier groups motivating this PR
(`sw_metricsHour` at 15d, etc.) match this profile exactly.
--
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]
