Copilot commented on code in PR #1046:
URL:
https://github.com/apache/skywalking-banyandb/pull/1046#discussion_r3035616979
##########
banyand/internal/storage/tsdb_test.go:
##########
@@ -308,6 +309,55 @@ func TestTakeFileSnapshot(t *testing.T) {
require.NoError(t, tsdb.Close())
})
+
+ t.Run("Take snapshot skips shard with no current snapshot", func(t
*testing.T) {
+ dir, defFn := test.Space(require.New(t))
+ defer defFn()
+
+ snapshotDir := filepath.Join(dir, "snapshot")
+
+ opts := TSDBOpts[*SnapshotMockTSTable, any]{
+ Location: dir,
+ SegmentInterval: IntervalRule{Unit: DAY, Num: 1},
+ TTL: IntervalRule{Unit: DAY, Num: 7},
+ ShardNum: 1,
+ TSTableCreator: SnapshotMockTSTableCreator,
+ }
+
+ ctx := context.Background()
+ mc := timestamp.NewMockClock()
+ ts, err := time.ParseInLocation("2006-01-02 15:04:05",
"2024-05-01 00:00:00", time.Local)
+ require.NoError(t, err)
+ mc.Set(ts)
+ ctx = timestamp.SetClock(ctx, mc)
+
+ serviceCache := NewServiceCache()
+ tsdb, err := OpenTSDB(ctx, opts, serviceCache, group)
+ require.NoError(t, err)
+ defer tsdb.Close()
+
+ normalSeg, err := tsdb.CreateSegmentIfNotExist(ts)
+ require.NoError(t, err)
+ normalSeg.DecRef()
+
+ epochSeg, err := tsdb.CreateSegmentIfNotExist(time.Unix(0, 0))
+ require.NoError(t, err)
+ epochSeg.DecRef()
+
+ created, snapshotErr := tsdb.TakeFileSnapshot(snapshotDir)
+ require.NoError(t, snapshotErr,
+ "snapshot should not fail due to empty shard in epoch
segment")
+ require.True(t, created)
+
+ normalSegDir := filepath.Join(snapshotDir,
+ fmt.Sprintf("seg-%s", ts.Format("20060102")))
+ require.DirExists(t, normalSegDir,
+ "normal segment should be present in snapshot")
+
+ epochSegDir := filepath.Join(snapshotDir, "seg-19700101")
+ require.DirExists(t, epochSegDir,
+ "epoch segment directory should still be created")
Review Comment:
This test builds segment snapshot paths using hard-coded formatting
("seg-%s" + dayFormat). To make the test resilient to future naming/format
changes, prefer deriving the expected directory name from the created segment’s
location (like the earlier test does) or reuse the package’s existing segment
naming helpers/constants.
##########
banyand/internal/storage/tsdb.go:
##########
@@ -309,6 +309,11 @@ func (d *database[T, O]) TakeFileSnapshot(dst string)
(bool, error) {
shardPath := filepath.Join(segPath, shardDir)
d.lfs.MkdirIfNotExist(shardPath, DirPerm)
if _, err := shard.table.TakeFileSnapshot(shardPath);
err != nil {
+ if errors.Is(err, ErrNoCurrentSnapshot) {
+ log.Warn().Str("shard",
shardDir).Str("segment", segDir).
Review Comment:
The new warning for ErrNoCurrentSnapshot can become noisy in normal
operation (e.g., newly created/empty shards during snapshot). Consider lowering
this to Debug (or Info) to avoid log spam, since this condition is now treated
as expected/non-fatal.
```suggestion
log.Debug().Str("shard",
shardDir).Str("segment", segDir).
```
##########
CHANGES.md:
##########
@@ -19,6 +19,7 @@ Release Notes.
- MCP: Add validation for properties and harden the mcp server.
- Fix property schema client connection not stable after data node restarted.
- Fix flaky on-disk integration tests caused by Ginkgo v2 random container
shuffling closing gRPC connections prematurely.
+- Fix take snapshot error when no data in the segment.
Review Comment:
Release note wording is a bit ungrammatical ("Fix take snapshot error when
no data in the segment"). Consider rephrasing to something like "Fix snapshot
error when there is no data in a segment" for clarity.
```suggestion
- Fix snapshot error when there is no data in a segment.
```
--
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]
