Copilot commented on code in PR #992:
URL:
https://github.com/apache/skywalking-banyandb/pull/992#discussion_r2894079811
##########
banyand/stream/snapshot.go:
##########
@@ -221,13 +222,17 @@ func (tst *tsTable) TakeFileSnapshot(dst string) error {
destPartPath := filepath.Join(dst, filepath.Base(srcPath))
if err := tst.fileSystem.CreateHardLink(srcPath, destPartPath,
nil); err != nil {
- return fmt.Errorf("failed to create snapshot for part
%d: %w", part.partMetadata.ID, err)
+ return false, fmt.Errorf("failed to create snapshot for
part %d: %w", part.partMetadata.ID, err)
}
+ hasDiskParts = true
+ }
+ if !hasDiskParts {
+ return false, nil
Review Comment:
`created` can be returned as `false` even though this method has already
created snapshot artifacts (e.g., it snapshots the index into `dst` before
checking whether there are any on-disk parts). That makes the boolean
misleading and can leave partially-created snapshot directories behind when
callers treat `created == false` as “nothing happened”. Consider determining
whether there is anything to snapshot before writing to `dst`, or treat
index-only snapshots as `created == true`, or clean up any created dirs/files
when returning `false`.
```suggestion
// An index snapshot has already been created; treat this as a
created snapshot
return true, nil
```
##########
banyand/measure/svc_data.go:
##########
@@ -384,7 +384,7 @@ func (s *dataSVC) takeGroupSnapshot(dstDir string,
groupName string) error {
return errors.Errorf("group %s has no tsdb",
group.GetSchema().Metadata.Name)
}
tsdb := db.(storage.TSDB[*tsTable, option])
- if err := tsdb.TakeFileSnapshot(dstDir); err != nil {
+ if _, err := tsdb.TakeFileSnapshot(dstDir); err != nil {
return errors.WithMessagef(err, "snapshot %s fail to take file
snapshot for group %s", dstDir, group.GetSchema().Metadata.Name)
}
Review Comment:
This call ignores the new `created` return from `TSDB.TakeFileSnapshot`.
When a group has no segments, `TakeFileSnapshot` now returns `(false, nil)` and
does not create the snapshot directory; the snapshot listener in this service
will still report a successful snapshot name, and the backup command will fail
when it tries to walk a non-existent snapshot directory. Consider propagating
`created` up (e.g., make `takeGroupSnapshot` return `(bool, error)` like the
standalone services) and have the snapshot listener skip/return `nil` when
nothing was created and no errors occurred.
```suggestion
created, err := tsdb.TakeFileSnapshot(dstDir)
if err != nil {
return errors.WithMessagef(err, "snapshot %s fail to take file
snapshot for group %s", dstDir, group.GetSchema().Metadata.Name)
}
if !created {
return errors.Errorf("snapshot %s was not created for group %s
(no segments)", dstDir, group.GetSchema().Metadata.Name)
}
```
##########
CHANGES.md:
##########
@@ -25,6 +25,7 @@ Release Notes.
- Add a generic snapshot coordination package for atomic snapshot transitions
across trace and sidx.
- Support map-reduce aggregation for measure queries: map phase (partial
aggregation on data nodes) and reduce phase (final aggregation on liaison).
- Add eBPF-based KTM I/O monitor for FODC agent.
+* Ignore take snapshot when no data.
Review Comment:
The new entry uses `*` while the rest of the file consistently uses `-` for
list items in this section. Please keep the bullet style consistent to avoid
formatting inconsistencies in rendered release notes.
```suggestion
- Ignore take snapshot when no data.
```
##########
banyand/trace/snapshot.go:
##########
@@ -241,13 +242,17 @@ func (tst *tsTable) TakeFileSnapshot(dst string) error {
destPartPath := filepath.Join(dst, filepath.Base(srcPath))
if err := tst.fileSystem.CreateHardLink(srcPath, destPartPath,
nil); err != nil {
- return fmt.Errorf("failed to create snapshot for part
%d: %w", part.partMetadata.ID, err)
+ return false, fmt.Errorf("failed to create snapshot for
part %d: %w", part.partMetadata.ID, err)
}
+ hasDiskParts = true
+ }
+ if !hasDiskParts {
+ return false, nil
}
Review Comment:
`created` can be returned as `false` even though this method has already
created snapshot artifacts (it snapshots `tst.sidxMap` into `dst` before
checking whether there are any on-disk parts). This makes the boolean
misleading and can leave partially-created snapshot directories behind when
callers treat `created == false` as “no snapshot”. Consider checking whether
there is anything to snapshot before creating index snapshots/directories, or
cleaning up `dst` on the `false, nil` return path.
--
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]
