Copilot commented on code in PR #955:
URL:
https://github.com/apache/skywalking-banyandb/pull/955#discussion_r2715364295
##########
banyand/property/listener.go:
##########
@@ -234,30 +234,38 @@ func (s *snapshotListener) Rev(ctx context.Context,
message bus.Message) bus.Mes
defer s.snapshotMux.Unlock()
storage.DeleteStaleSnapshots(s.s.snapshotDir, s.s.maxFileSnapshotNum,
s.s.minFileSnapshotAge, s.s.lfs)
sn := s.snapshotName()
- shardsRef := s.s.db.sLst.Load()
- if shardsRef == nil {
- return bus.NewMessage(bus.MessageID(time.Now().UnixNano()), nil)
- }
- shards := *shardsRef
- for _, shard := range shards {
- select {
- case <-ctx.Done():
- return
bus.NewMessage(bus.MessageID(time.Now().UnixNano()), nil)
- default:
+ var snapshotResult *databasev1.Snapshot
+ s.s.db.groups.Range(func(_, value any) bool {
+ gs := value.(*groupShards)
+ sLst := gs.shards.Load()
+ if sLst == nil {
+ return true
}
- snpDir := path.Join(s.s.snapshotDir, sn, storage.DataDir,
filepath.Base(shard.location))
- lfs.MkdirPanicIfExist(snpDir, storage.DirPerm)
- err := shard.store.TakeFileSnapshot(snpDir)
- if err != nil {
- s.s.l.Error().Err(err).Str("shard",
filepath.Base(shard.location)).Msg("fail to take shard snapshot")
- return
bus.NewMessage(bus.MessageID(time.Now().UnixNano()), &databasev1.Snapshot{
- Name: sn,
- Catalog: commonv1.Catalog_CATALOG_PROPERTY,
- Error: err.Error(),
- })
+ for _, shardRef := range *sLst {
+ select {
+ case <-ctx.Done():
Review Comment:
If the snapshot context is cancelled, the callback returns false and stops
iteration, but the function still falls through and returns a successful
Snapshot (with no Error). This changes behavior vs the previous early-return on
ctx.Done() and can report a snapshot that was never fully created. Consider
tracking ctx.Err() (or setting snapshotResult to an error Snapshot) and
returning early/with error when ctx is cancelled.
```suggestion
case <-ctx.Done():
// Context cancelled: record an error snapshot
and stop iteration.
if err := ctx.Err(); err != nil {
snapshotResult = &databasev1.Snapshot{
Name: sn,
Catalog:
commonv1.Catalog_CATALOG_PROPERTY,
Error: err.Error(),
}
}
```
##########
CHANGES.md:
##########
@@ -19,6 +19,9 @@ Release Notes.
- Add replication integration test for measure.
- Activate the property repair mechanism by default.
- Add snapshot time retention policy to ensure the snapshot only can be
deleted after the configured minimum age(time).
+- **Breaking Change**: Change the data storage path structure for property
model:
+ - From: `<data-dir>/property/shard-<id>/...`
+ - To: `<data-dir>/property/<group>/shard-<id>/...`
Review Comment:
The breaking-change paths here appear to miss the `data` segment. The
property service opens the DB at `<root>/property/data` (see
banyand/property/service.go:147), so the old/new shard paths are
`<data-dir>/property/data/shard-<id>/...` ->
`<data-dir>/property/data/<group>/shard-<id>/...`. Updating this avoids
confusing operators during upgrade/migration.
```suggestion
- From: `<data-dir>/property/data/shard-<id>/...`
- To: `<data-dir>/property/data/<group>/shard-<id>/...`
```
--
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]
