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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
