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/08 02:37:26 UTC
[PR] Fix lifecycle migration segment shorter than the configured `SegmentInterval` [skywalking-banyandb]
mrproliu opened a new pull request, #1120:
URL: https://github.com/apache/skywalking-banyandb/pull/1120
## What
Inspector reports cold-tier segments shorter than the configured
`SegmentInterval` on multi-stage clusters — e.g. a 3-day segment under a
15-day `SegmentInterval` for `sw_metricsHour`.
## Why
Two independent bugs:
1. **Receiver**: `segment.create()` used `IntervalUnit.Standard`, which
aligned `start` only to day / hour boundaries — `Num` was ignored.
Out-of-order chunked-sync arrivals during lifecycle migration could
create new segments that got truncated by an existing later neighbour,
producing a span shorter than configured.
2. **Sender**: `getTargetStageInterval` always returned `Stages[0]`. On
3+ stage clusters this fed the wrong (current-stage) interval into
target bucket math and progress keys for non-first-stage migrations.
## How
- New `IntervalRule.Standard(t)` aligns to a `Num*Unit`-from-epoch grid in
UTC and returns the result in `t.Location()`. `segment.create` and
`wqueue.GetTimeRange` now use it.
- `segment.create` bumps the aligned `start` past any legacy off-grid
neighbour so writes are never routed into a segment whose nominal range
no longer covers them.
- `GroupConfig.TargetSegmentInterval` carries the actual next-stage interval,
populated by `parseGroup` and read by `getTargetStageInterval`.
## Compatibility
Existing data is untouched. Legacy off-grid segments continue to function and
age out via TTL. A new `INFO` log surfaces the transition-period case where
a new segment is forced shorter than its configured `SegmentInterval` due to
a legacy neighbour — self-heals once the neighbour ages out.
## Tests
- `IntervalRule.Standard`: epoch-grid alignment, location preservation,
pre-epoch floor division.
- `segment.create`: same / adjacent bucket out-of-order arrival, hour
intervals, 32-goroutine concurrent alignment, legacy-neighbour
no-misroute, persisted metadata round-trip.
- Lifecycle: `getTargetStageInterval` table-driven across 2 / 3 / 4-stage
configs and all fallback paths; 64-goroutine concurrent reader.
- Updated `rotation_test.go` and `wqueue_test.go` assertions to align with
the new grid behavior.
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Fixes apache/skywalking#<issue number>.
- [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 lifecycle migration segment shorter than the configured `SegmentInterval` [skywalking-banyandb]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1120:
URL: https://github.com/apache/skywalking-banyandb/pull/1120#discussion_r3206054508
##########
banyand/internal/storage/segment_test.go:
##########
@@ -1178,3 +1180,455 @@ func TestSegment_ConcurrentReopenAndClose_NoPanic(t *testing.T) {
require.Nil(t, s.index, "segment %s should be cleaned up", s.suffix)
}
}
+
+// newAlignmentTestController is a compact helper for tests covering the
+// epoch-aligned segment.create() behavior introduced by the lifecycle fix.
+// It spins up a real segmentController backed by a temp dir, with the given
+// SegmentInterval, and returns the controller plus a cleanup function.
+func newAlignmentTestController(
+ t *testing.T, interval IntervalRule,
+) (*segmentController[mockTSTable, mockTSTableOpener], string, func()) {
+ t.Helper()
+ tempDir, cleanup := setupTestEnvironment(t)
+
+ ctx := context.Background()
+ l := logger.GetLogger("test-alignment")
+ ctx = context.WithValue(ctx, logger.ContextKey, l)
+ ctx = common.SetPosition(ctx, func(_ common.Position) common.Position {
+ return common.Position{
+ Database: "test-db",
+ Stage: "alignment",
+ }
+ })
+
+ opts := TSDBOpts[mockTSTable, mockTSTableOpener]{
+ TSTableCreator: func(_ fs.FileSystem, _ string, _ common.Position, _ *logger.Logger,
+ _ timestamp.TimeRange, _ mockTSTableOpener, _ any,
+ ) (mockTSTable, error) {
+ return mockTSTable{ID: common.ShardID(0)}, nil
+ },
+ ShardNum: 1,
+ SegmentInterval: interval,
+ TTL: IntervalRule{Unit: DAY, Num: 60},
+ SeriesIndexFlushTimeoutSeconds: 10,
+ SeriesIndexCacheMaxBytes: 1024 * 1024,
+ }
+
+ serviceCache := NewServiceCache().(*serviceCache)
+ sc := newSegmentController[mockTSTable, mockTSTableOpener](
+ ctx,
+ tempDir,
+ l,
+ opts,
+ nil,
+ nil,
+ 5*time.Minute,
+ fs.NewLocalFileSystemWithLoggerAndLimit(logger.GetLogger("storage"), opts.MemoryLimit),
+ serviceCache,
+ group,
+ )
+ return sc, tempDir, cleanup
+}
+
+// TestIntervalRule_Standard_AlignsToEpochGrid pins the alignment math itself.
+// Two timestamps in the same N*Unit bucket measured from epoch must round
+// down to the exact same instant; two timestamps in adjacent buckets must
+// round to instants exactly N*Unit apart.
+func TestIntervalRule_Standard_AlignsToEpochGrid(t *testing.T) {
+ //nolint:govet // fieldalignment: test struct optimization not critical
+ cases := []struct {
+ name string
+ ir IntervalRule
+ probes []time.Time
+ expected []time.Time
+ }{
+ {
+ name: "DAY Num=15 matches calculateTargetSegments grid",
+ ir: IntervalRule{Unit: DAY, Num: 15},
+ probes: []time.Time{
+ time.Date(2026, 4, 7, 0, 0, 0, 0, time.UTC),
+ time.Date(2026, 4, 16, 6, 0, 0, 0, time.UTC),
+ time.Date(2026, 4, 19, 23, 59, 59, 0, time.UTC),
+ time.Date(2026, 4, 22, 0, 0, 0, 0, time.UTC),
+ time.Date(2026, 5, 7, 12, 30, 0, 0, time.UTC),
+ },
+ expected: []time.Time{
+ time.Date(2026, 4, 7, 0, 0, 0, 0, time.UTC),
+ time.Date(2026, 4, 7, 0, 0, 0, 0, time.UTC),
+ time.Date(2026, 4, 7, 0, 0, 0, 0, time.UTC),
+ time.Date(2026, 4, 22, 0, 0, 0, 0, time.UTC),
+ time.Date(2026, 5, 7, 0, 0, 0, 0, time.UTC),
+ },
+ },
+ {
+ name: "DAY Num=5",
+ ir: IntervalRule{Unit: DAY, Num: 5},
+ // 1970-01-01 + N*5 day boundaries near today: 2026-04-02 (day 20545),
+ // 2026-04-07 (day 20550), 2026-04-12 (day 20555).
+ probes: []time.Time{
+ time.Date(2026, 4, 2, 0, 0, 0, 0, time.UTC),
+ time.Date(2026, 4, 3, 6, 0, 0, 0, time.UTC),
+ time.Date(2026, 4, 6, 23, 0, 0, 0, time.UTC),
+ time.Date(2026, 4, 7, 0, 0, 0, 0, time.UTC),
+ },
+ expected: []time.Time{
+ time.Date(2026, 4, 2, 0, 0, 0, 0, time.UTC),
+ time.Date(2026, 4, 2, 0, 0, 0, 0, time.UTC),
+ time.Date(2026, 4, 2, 0, 0, 0, 0, time.UTC),
+ time.Date(2026, 4, 7, 0, 0, 0, 0, time.UTC),
+ },
+ },
+ {
+ name: "HOUR Num=6",
+ ir: IntervalRule{Unit: HOUR, Num: 6},
+ probes: []time.Time{
+ time.Date(2026, 4, 19, 0, 0, 0, 0, time.UTC),
+ time.Date(2026, 4, 19, 5, 59, 59, 0, time.UTC),
+ time.Date(2026, 4, 19, 6, 0, 0, 0, time.UTC),
+ time.Date(2026, 4, 19, 11, 30, 0, 0, time.UTC),
+ time.Date(2026, 4, 19, 12, 0, 0, 0, time.UTC),
+ },
+ expected: []time.Time{
+ time.Date(2026, 4, 19, 0, 0, 0, 0, time.UTC),
+ time.Date(2026, 4, 19, 0, 0, 0, 0, time.UTC),
+ time.Date(2026, 4, 19, 6, 0, 0, 0, time.UTC),
+ time.Date(2026, 4, 19, 6, 0, 0, 0, time.UTC),
+ time.Date(2026, 4, 19, 12, 0, 0, 0, time.UTC),
+ },
+ },
+ {
+ name: "DAY Num=1 matches old Unit.Standard behavior",
+ ir: IntervalRule{Unit: DAY, Num: 1},
+ probes: []time.Time{time.Date(2026, 4, 19, 6, 30, 0, 0, time.UTC)},
+ expected: []time.Time{time.Date(2026, 4, 19, 0, 0, 0, 0, time.UTC)},
+ },
+ {
+ name: "Num=0 falls back to Unit.Standard",
+ ir: IntervalRule{Unit: DAY, Num: 0},
+ probes: []time.Time{time.Date(2026, 4, 19, 6, 30, 0, 0, time.UTC)},
+ expected: []time.Time{time.Date(2026, 4, 19, 0, 0, 0, 0, time.UTC)},
+ },
+ {
+ // Pre-epoch inputs: floor division puts them in the bucket below
+ // rather than collapsing to epoch as truncated division would.
+ name: "DAY Num=15 pre-epoch uses floor division",
+ ir: IntervalRule{Unit: DAY, Num: 15},
+ probes: []time.Time{
+ time.Date(1969, 12, 25, 0, 0, 0, 0, time.UTC),
+ time.Date(1969, 12, 31, 23, 59, 59, 0, time.UTC),
+ },
+ expected: []time.Time{
+ time.Date(1969, 12, 17, 0, 0, 0, 0, time.UTC),
+ time.Date(1969, 12, 17, 0, 0, 0, 0, time.UTC),
+ },
+ },
+ }
+ for _, tc := range cases {
+ t.Run(tc.name, func(t *testing.T) {
+ require.Equal(t, len(tc.probes), len(tc.expected))
+ for i, p := range tc.probes {
+ got := tc.ir.Standard(p)
+ assert.Equal(t, tc.expected[i], got,
+ "Standard(%s) under %d %s expected %s got %s",
+ p.Format(time.RFC3339), tc.ir.Num, tc.ir.Unit.String(),
+ tc.expected[i].Format(time.RFC3339), got.Format(time.RFC3339))
+ }
+ })
+ }
+}
+
+// TestIntervalRule_Standard_PreservesLocation verifies that the returned
+// time keeps the input's Location while the bucket instant stays invariant
+// across timezones - the grid is anchored in UTC for cross-node consistency
+// but presented in the caller's local time.
+func TestIntervalRule_Standard_PreservesLocation(t *testing.T) {
+ shanghai := time.FixedZone("CST", 8*3600)
+ losAngeles := time.FixedZone("PST", -8*3600)
+
+ cases := []struct {
+ probe time.Time
+ ir IntervalRule
+ }{
+ {time.Date(2026, 4, 19, 6, 0, 0, 0, time.UTC), IntervalRule{Unit: DAY, Num: 15}},
+ {time.Date(2026, 4, 19, 11, 30, 0, 0, time.UTC), IntervalRule{Unit: HOUR, Num: 6}},
+ }
+ for _, c := range cases {
+ want := c.ir.Standard(c.probe)
+ for _, loc := range []*time.Location{time.UTC, shanghai, losAngeles} {
+ got := c.ir.Standard(c.probe.In(loc))
+ assert.Equal(t, loc, got.Location(),
+ "Standard must return time in input.Location(); ir=%+v loc=%s", c.ir, loc)
+ assert.True(t, got.Equal(want),
+ "bucket instant must be timezone-invariant; ir=%+v loc=%s utc=%s got=%s",
+ c.ir, loc, want.Format(time.RFC3339), got.Format(time.RFC3339))
+ }
+ }
+}
+
+// TestCreateSegment_OutOfOrderArrival_SameBucket reproduces the production
+// truncation observed on demo-banyandb-data-cold-0 on 2026-04-30 where a part
+// with MinTimestamp ~2026-04-19 created seg-20260419 first and a later-
+// processed part with MinTimestamp ~2026-04-16 produced a 3-day truncated
+// seg-20260416. With Standard() honoring Num, both timestamps resolve to the
+// same epoch-aligned bucket [04/07, 04/22), so the second create returns the
+// same segment instead of creating a truncated neighbor.
+func TestCreateSegment_OutOfOrderArrival_SameBucket(t *testing.T) {
+ sc, _, cleanup := newAlignmentTestController(t, IntervalRule{Unit: DAY, Num: 15})
+ defer cleanup()
+
+ laterFirst := time.Date(2026, 4, 19, 6, 0, 0, 0, time.UTC)
+ earlierAfter := time.Date(2026, 4, 16, 6, 0, 0, 0, time.UTC)
+ expectedBucketStart := time.Date(2026, 4, 7, 0, 0, 0, 0, time.UTC)
+ expectedBucketEnd := time.Date(2026, 4, 22, 0, 0, 0, 0, time.UTC)
+
+ seg1, err := sc.create(laterFirst)
+ require.NoError(t, err)
+ require.NotNil(t, seg1)
+ defer seg1.DecRef()
+
+ seg2, err := sc.create(earlierAfter)
+ require.NoError(t, err)
+ require.NotNil(t, seg2)
+
+ assert.Same(t, seg1, seg2,
+ "both timestamps fall in the same 15d bucket; create() must reuse the existing segment")
+ assert.Equal(t, expectedBucketStart, seg1.Start,
+ "segment must start at the epoch-aligned bucket boundary, not at the first arrival's day")
+ assert.Equal(t, expectedBucketEnd, seg1.End)
+ assert.Equal(t, 15*24*time.Hour, seg1.End.Sub(seg1.Start))
+}
+
+// TestCreateSegment_OutOfOrderArrival_AdjacentBuckets covers the case where
+// the late-arrival timestamp falls in a strictly different epoch bucket from
+// the first-arrival one. The new segment must occupy a clean N*Unit span
+// without being truncated by the existing later segment - the truncation
+// branch in create() must collapse to "stdEnd == next.Start" exactly, leaving
+// span = N*Unit.
+func TestCreateSegment_OutOfOrderArrival_AdjacentBuckets(t *testing.T) {
+ sc, _, cleanup := newAlignmentTestController(t, IntervalRule{Unit: DAY, Num: 15})
+ defer cleanup()
+
+ laterFirst := time.Date(2026, 5, 5, 6, 0, 0, 0, time.UTC) // bucket [04/22, 05/07)
+ earlierAfter := time.Date(2026, 4, 15, 6, 0, 0, 0, time.UTC) // bucket [04/07, 04/22)
+
+ seg1, err := sc.create(laterFirst)
+ require.NoError(t, err)
+ require.NotNil(t, seg1)
+ defer seg1.DecRef()
+ assert.Equal(t, time.Date(2026, 4, 22, 0, 0, 0, 0, time.UTC), seg1.Start)
+ assert.Equal(t, time.Date(2026, 5, 7, 0, 0, 0, 0, time.UTC), seg1.End)
+ assert.Equal(t, 15*24*time.Hour, seg1.End.Sub(seg1.Start))
+
+ seg2, err := sc.create(earlierAfter)
+ require.NoError(t, err)
+ require.NotNil(t, seg2)
+ defer seg2.DecRef()
+ assert.NotSame(t, seg1, seg2,
+ "timestamps in different buckets must not collapse to one segment")
+ assert.Equal(t, time.Date(2026, 4, 7, 0, 0, 0, 0, time.UTC), seg2.Start)
+ assert.Equal(t, time.Date(2026, 4, 22, 0, 0, 0, 0, time.UTC), seg2.End,
+ "new earlier segment must abut next.Start with stdEnd==next.Start (no internal truncation)")
+ assert.Equal(t, 15*24*time.Hour, seg2.End.Sub(seg2.Start))
+}
+
+// TestCreateSegment_HourInterval_OutOfOrder verifies the same alignment
+// guarantee for HOUR-based segment intervals (Num=6).
+func TestCreateSegment_HourInterval_OutOfOrder(t *testing.T) {
+ sc, _, cleanup := newAlignmentTestController(t, IntervalRule{Unit: HOUR, Num: 6})
+ defer cleanup()
+
+ t1 := time.Date(2026, 4, 19, 11, 30, 0, 0, time.UTC) // bucket [06:00, 12:00)
+ t2 := time.Date(2026, 4, 19, 8, 15, 0, 0, time.UTC) // same bucket
+ t3 := time.Date(2026, 4, 19, 13, 0, 0, 0, time.UTC) // bucket [12:00, 18:00)
+
+ seg1, err := sc.create(t1)
+ require.NoError(t, err)
+ defer seg1.DecRef()
+ assert.Equal(t, time.Date(2026, 4, 19, 6, 0, 0, 0, time.UTC), seg1.Start)
+ assert.Equal(t, 6*time.Hour, seg1.End.Sub(seg1.Start))
+
+ seg2, err := sc.create(t2)
+ require.NoError(t, err)
+ assert.Same(t, seg1, seg2, "08:15 must reuse the [06:00, 12:00) segment")
+
+ seg3, err := sc.create(t3)
+ require.NoError(t, err)
+ defer seg3.DecRef()
+ assert.NotSame(t, seg1, seg3)
+ assert.Equal(t, time.Date(2026, 4, 19, 12, 0, 0, 0, time.UTC), seg3.Start)
+ assert.Equal(t, 6*time.Hour, seg3.End.Sub(seg3.Start))
+}
+
+// TestCreateSegment_ConcurrentCreates_DeterministicAlignment exercises the
+// receiver under contention: many goroutines call create() with random
+// timestamps spread across a window that covers multiple epoch-aligned
+// buckets. Whatever interleaving the scheduler picks, the resulting segment
+// set must (a) cover every probe timestamp, (b) all start on epoch-aligned
+// boundaries, (c) every span be exactly the configured N*Unit, and (d) be
+// non-overlapping. This pins down the property that motivated the fix:
+// alignment is determined by the global grid, not by arrival order.
+func TestCreateSegment_ConcurrentCreates_DeterministicAlignment(t *testing.T) {
+ sc, _, cleanup := newAlignmentTestController(t, IntervalRule{Unit: DAY, Num: 15})
+ defer cleanup()
+
+ // Probe range covers 4 epoch-aligned 15d buckets:
+ // [03/08, 03/23), [03/23, 04/07), [04/07, 04/22), [04/22, 05/07).
+ rangeStart := time.Date(2026, 3, 10, 0, 0, 0, 0, time.UTC)
+ rangeEndExclusive := time.Date(2026, 5, 5, 0, 0, 0, 0, time.UTC)
+ rangeNanos := rangeEndExclusive.UnixNano() - rangeStart.UnixNano()
+
+ const goroutines = 32
+ const probesPerGoroutine = 64
+ const knuthMul = uint64(0x9E3779B97F4A7C15)
+ probes := make([]time.Time, 0, goroutines*probesPerGoroutine)
+ for i := 0; i < goroutines*probesPerGoroutine; i++ {
+ offset := int64((uint64(i+1)*knuthMul)>>1) % rangeNanos
+ probes = append(probes, time.Unix(0, rangeStart.UnixNano()+offset).UTC())
+ }
+
+ var wg sync.WaitGroup
+ wg.Add(goroutines)
+ for g := 0; g < goroutines; g++ {
+ go func(idx int) {
+ defer wg.Done()
+ start := idx * probesPerGoroutine
+ end := start + probesPerGoroutine
+ for _, ts := range probes[start:end] {
+ seg, err := sc.create(ts)
+ if err != nil {
+ t.Errorf("create(%s) returned error: %v", ts.Format(time.RFC3339), err)
+ return
+ }
+ if seg == nil {
+ t.Errorf("create(%s) returned nil segment", ts.Format(time.RFC3339))
+ return
+ }
+ if !ts.Before(seg.Start.Add(15*24*time.Hour)) || ts.Before(seg.Start) {
+ t.Errorf("probe %s landed in segment [%s, %s) which does not contain it",
+ ts.Format(time.RFC3339), seg.Start.Format(time.RFC3339), seg.End.Format(time.RFC3339))
+ }
+ seg.DecRef()
Review Comment:
The probe containment assertion uses ts.Before(seg.Start.Add(15*24*time.Hour)) rather than seg.End/seg.Contains(...). If create() ever returns a segment whose span is shorter than 15d (e.g., the legacy-neighbor transition case this PR introduces), this check can produce false positives/negatives and miss real misrouting; assert against seg.End (or seg.Contains) instead of a hard-coded duration from seg.Start.
##########
banyand/backup/lifecycle/steps.go:
##########
@@ -130,14 +157,21 @@ func parseGroup(
return nil, nil
}
nst = ro.Stages[i+1]
- segmentInterval = st.SegmentInterval
- l.Info().Msgf("migrating group %s at stage %s to stage %s, segment interval: %d(%s), total ttl needs: %d(%s)",
- g.Metadata.Name, st.Name, nst.Name, segmentInterval.Num, segmentInterval.Unit.String(), ttlTime.Num, ttlTime.Unit.String())
+ // Clone before exposing through GroupConfig so callers cannot mutate
+ // the shared proto Stages[*] sub-objects.
+ segmentInterval = cloneIntervalRule(st.SegmentInterval)
+ targetSegmentInterval = cloneIntervalRule(nst.SegmentInterval)
+ l.Info().Msgf("migrating group %s at stage %s to stage %s, source segment interval: %d(%s), target segment interval: %d(%s), total ttl needs: %d(%s)",
+ g.Metadata.Name, st.Name, nst.Name,
+ segmentInterval.Num, segmentInterval.Unit.String(),
Review Comment:
In the matched-stage path, nst := ro.Stages[i+1] is used to populate/log targetSegmentInterval, but nst.SegmentInterval is not validated before cloneIntervalRule() and targetSegmentInterval.Num/Unit.String() are dereferenced. If the next stage is missing segment_interval, this will panic instead of returning a validation error; validate nst.SegmentInterval (and avoid dereferencing when nil) before logging/returning GroupConfig.
##########
banyand/backup/lifecycle/steps.go:
##########
@@ -113,10 +130,20 @@ func parseGroup(
if len(ro.Stages) == 0 {
return nil, fmt.Errorf("no stages in group %s", g.Metadata.Name)
}
+ if ro.Ttl == nil {
+ return nil, fmt.Errorf("group %s: missing ttl", g.Metadata.Name)
+ }
Review Comment:
parseGroup clones ro.SegmentInterval into segmentInterval but doesn't validate that ro.SegmentInterval is non-nil. segmentInterval is later dereferenced in the defaulting log (and returned in GroupConfig), so an invalid Group config could still trigger a nil-pointer panic here; consider validating ro.SegmentInterval (similar to ro.Ttl) and returning a descriptive error.
--
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 lifecycle migration segment shorter than the configured `SegmentInterval` [skywalking-banyandb]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1120:
URL: https://github.com/apache/skywalking-banyandb/pull/1120#discussion_r3205958719
##########
banyand/internal/storage/segment.go:
##########
@@ -592,6 +598,18 @@ func (sc *segmentController[T, O]) create(start time.Time) (*segment[T, O], erro
stdEnd := options.SegmentInterval.NextTime(start)
var end time.Time
Review Comment:
In `create`, `stdEnd` is computed after `start` may be bumped to `s.End` when alignment lands inside a legacy off-grid segment. If there is no `next` segment yet, this will create a full-length segment starting at the legacy segment’s end (off the epoch grid), which can permanently steer subsequent segment creation away from the intended Num*Unit-from-epoch boundaries. Consider computing `alignedStart := SegmentInterval.Standard(raw)` and `stdEnd := SegmentInterval.NextTime(alignedStart)` before any bump, then only moving `start` forward to avoid overlap while keeping `end` capped at the original `stdEnd` (or `min(stdEnd, next.Start)`), so the transition segment is shortened but still ends on the global grid. Adding a test for the “legacy neighbor, no next segment exists yet” case would help prevent regressions.
##########
banyand/backup/lifecycle/steps.go:
##########
@@ -130,14 +148,21 @@ func parseGroup(
return nil, nil
}
nst = ro.Stages[i+1]
- segmentInterval = st.SegmentInterval
- l.Info().Msgf("migrating group %s at stage %s to stage %s, segment interval: %d(%s), total ttl needs: %d(%s)",
- g.Metadata.Name, st.Name, nst.Name, segmentInterval.Num, segmentInterval.Unit.String(), ttlTime.Num, ttlTime.Unit.String())
+ // Clone before exposing through GroupConfig so callers cannot mutate
+ // the shared proto Stages[*] sub-objects.
+ segmentInterval = cloneIntervalRule(st.SegmentInterval)
+ targetSegmentInterval = cloneIntervalRule(nst.SegmentInterval)
+ l.Info().Msgf("migrating group %s at stage %s to stage %s, source segment interval: %d(%s), target segment interval: %d(%s), total ttl needs: %d(%s)",
+ g.Metadata.Name, st.Name, nst.Name,
+ segmentInterval.Num, segmentInterval.Unit.String(),
+ targetSegmentInterval.Num, targetSegmentInterval.Unit.String(),
+ ttlTime.Num, ttlTime.Unit.String())
Review Comment:
`segmentInterval` / `targetSegmentInterval` are dereferenced in the Info log message, but both can be nil if the group config is incomplete or unvalidated (e.g., missing stage.segment_interval). That would panic inside lifecycle migration instead of returning a clear error. Consider validating these fields and returning an error when either interval is nil/invalid, or logging them defensively (nil-safe) so misconfigurations don’t crash the process.
--
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 lifecycle migration segment shorter than the configured `SegmentInterval` [skywalking-banyandb]
Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily merged PR #1120:
URL: https://github.com/apache/skywalking-banyandb/pull/1120
--
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 lifecycle migration segment shorter than the configured `SegmentInterval` [skywalking-banyandb]
Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix lifecycle migration segment shorter than the configured `SegmentInterval` [skywalking-banyandb]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1120:
URL: https://github.com/apache/skywalking-banyandb/pull/1120#issuecomment-4402913279
## [Codecov](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1120?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 31.15%. 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 ([`1d4b1c2`](https://app.codecov.io/gh/apache/skywalking-banyandb/commit/1d4b1c239bb27545db678b026446eaee14f8a782?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 242 commits behind head on main.
> :exclamation: There is a different number of reports uploaded between BASE (3530dd9) and HEAD (1d4b1c2). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (3530dd9) | HEAD (1d4b1c2) |
>|------|------|------|
>||1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #1120 +/- ##
===========================================
- Coverage 45.97% 31.15% -14.83%
===========================================
Files 328 128 -200
Lines 55505 13692 -41813
===========================================
- Hits 25520 4266 -21254
+ Misses 27909 9014 -18895
+ Partials 2076 412 -1664
```
| [Flag](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1120/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/1120/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `31.15% <ø> (?)` | |
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/1120?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