Copilot commented on code in PR #950:
URL:
https://github.com/apache/skywalking-banyandb/pull/950#discussion_r2704437414
##########
banyand/internal/storage/snapshot.go:
##########
@@ -40,8 +57,22 @@ func DeleteStaleSnapshots(root string, maxNum int, lfs
fs.FileSystem) {
sort.Slice(snapshots, func(i, j int) bool {
return snapshots[i].Name() < snapshots[j].Name()
})
+ now := time.Now()
for i := 0; i < len(snapshots)-maxNum; i++ {
- lfs.MustRMAll(filepath.Join(root, snapshots[i].Name()))
+ snapshotName := snapshots[i].Name()
+ // If the min age not set, then only keep using the max num to
delete
Review Comment:
The comment has a grammatical error. It should read "If the min age is not
set" instead of "If the min age not set".
```suggestion
// If the min age is not set, then only keep using the max num
to delete
```
##########
banyand/internal/storage/snapshot.go:
##########
@@ -18,16 +18,33 @@
package storage
import (
+ "fmt"
"os"
"path/filepath"
"sort"
"time"
"github.com/apache/skywalking-banyandb/pkg/fs"
+ "github.com/apache/skywalking-banyandb/pkg/logger"
)
+const SnapshotTimeFormat = "20060102150405"
+
+// ParseSnapshotTimestamp extracts the creation time from a snapshot directory
name.
+func ParseSnapshotTimestamp(name string) (time.Time, error) {
+ if len(name) < 14 {
+ return time.Time{}, fmt.Errorf("snapshot name too short: %s",
name)
+ }
+ timestampStr := name[:14]
+ parsedTime, parseErr := time.Parse(SnapshotTimeFormat, timestampStr)
+ if parseErr != nil {
+ return time.Time{}, fmt.Errorf("failed to parse timestamp from
snapshot name %s: %w", name, parseErr)
+ }
+ return parsedTime, nil
+}
+
// DeleteStaleSnapshots deletes the stale snapshots in the root directory.
Review Comment:
The function documentation should clarify the interaction between maxNum and
minAge parameters. The current behavior is that minAge acts as a safety guard -
snapshots younger than minAge will never be deleted even if the count exceeds
maxNum. This important behavior should be documented in the function comment to
avoid confusion.
```suggestion
// DeleteStaleSnapshots deletes stale snapshots in the root directory based
on
// a maximum number of snapshots to retain and an optional minimum age.
//
// Snapshots are sorted by name (which encodes a timestamp) and the oldest
// snapshots are considered for deletion first. The parameter maxNum
specifies
// the desired maximum number of snapshots to keep; at most
len(snapshots)-maxNum
// of the oldest snapshots become candidates for deletion.
//
// The parameter minAge acts as a safety guard:
// - If minAge == 0, the candidates are deleted solely based on maxNum, so
// enough oldest snapshots are removed to leave at most maxNum snapshots.
// - If minAge > 0, a snapshot is deleted only if its age is at least
minAge.
// Snapshots younger than minAge are never deleted, even if the total
count
// of snapshots still exceeds maxNum.
```
##########
pkg/fs/file_system.go:
##########
@@ -124,6 +124,8 @@ type FileSystem interface {
MustGetTotalSpace(path string) uint64
// CreateHardLink creates hard links in destPath for files in srcPath
that pass the filter.
CreateHardLink(srcPath, destPath string, filter func(string) bool) error
+ // IsExist check the directory/file is exist or not.
Review Comment:
The comment has a grammatical error and missing word. It should read "check
if the directory/file exists or not" instead of "check the directory/file is
exist or not".
```suggestion
// IsExist checks if the directory/file exists or not.
```
##########
CHANGES.md:
##########
@@ -27,6 +28,7 @@ Release Notes.
- Fix unsupported empty string tag bug.
- Fix duplicate elements in stream query results by implementing element
ID-based deduplication across scan, merge, and result building stages.
- Fix data written to the wrong shard and related stream queries.
+- Fix the lifecycle got panic when the trace no sidx.
Review Comment:
The comment has a grammatical error. It should read "Fix the lifecycle panic
when the trace has no sidx" instead of "Fix the lifecycle got panic when the
trace no sidx".
```suggestion
- Fix the lifecycle panic when the trace has no sidx.
```
##########
banyand/backup/lifecycle/trace_migration_visitor.go:
##########
@@ -289,6 +289,10 @@ func (mv *traceMigrationVisitor) generateAllSidxPartData(
sourceShardID common.ShardID,
sidxPath string,
) ([]queue.StreamingPartData, []func(), error) {
+ // If the sidx not exist, then ignore for the life cycle
Review Comment:
The comment has a grammatical error. It should read "If the sidx does not
exist" instead of "If the sidx not exist".
```suggestion
// If the sidx does not exist, then ignore for the life cycle
```
--
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]
