Re: [PR] ignore backup if no segments in the storage [skywalking-banyandb]
hanahmily merged PR #992: URL: https://github.com/apache/skywalking-banyandb/pull/992 -- 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]
Re: [PR] ignore backup if no segments in the storage [skywalking-banyandb]
hanahmily commented on code in PR #992:
URL:
https://github.com/apache/skywalking-banyandb/pull/992#discussion_r2899219630
##
banyand/measure/snapshot.go:
##
@@ -370,16 +377,25 @@ func (s *snapshotListener) Rev(ctx context.Context,
message bus.Message) bus.Mes
}
groupName := g.GetSchema().Metadata.Name
snapshotPath := filepath.Join(s.s.snapshotDir, sn, groupName)
- if errGroup := s.s.takeGroupSnapshot(snapshotPath, groupName);
errGroup != nil {
+ created, errGroup := s.s.takeGroupSnapshot(snapshotPath,
groupName)
+ if errGroup != nil {
s.s.l.Error().Err(errGroup).Str("group",
groupName).Msg("fail to take group snapshot")
err = multierr.Append(err, errGroup)
continue
}
+ if !created {
+ s.s.l.Debug().Str("group", groupName).Msg("skip empty
group snapshot")
Review Comment:
```suggestion
s.s.l.Info().Str("group", groupName).Msg("skip empty
group snapshot")
```
##
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:
Move this line to a bug section.
--
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]
Re: [PR] ignore backup if no segments in the storage [skywalking-banyandb]
Copilot commented on code in PR #992:
URL:
https://github.com/apache/skywalking-banyandb/pull/992#discussion_r2896035653
##
banyand/trace/svc_standalone.go:
##
@@ -467,16 +469,25 @@ func (d *standaloneSnapshotListener) Rev(ctx
context.Context, message bus.Messag
}
groupName := g.GetSchema().Metadata.Name
snapshotPath := filepath.Join(d.s.snapshotDir, sn, groupName)
- if errGroup := d.s.takeGroupSnapshot(snapshotPath, groupName);
errGroup != nil {
+ created, errGroup := d.s.takeGroupSnapshot(snapshotPath,
groupName)
+ if errGroup != nil {
d.s.l.Error().Err(errGroup).Str("group",
groupName).Msg("fail to take group snapshot")
err = multierr.Append(err, errGroup)
continue
}
+ if !created {
+ d.s.l.Info().Str("group", groupName).Msg("skip empty
group snapshot")
+ continue
+ }
+ snapshotCreated++
// Compare snapshot with data directory to verify consistency
dataPath := filepath.Join(d.s.dataPath, groupName)
d.compareSnapshotWithData(snapshotPath, dataPath, groupName)
}
+ if snapshotCreated == 0 && err == nil {
+ return bus.NewMessage(bus.MessageID(time.Now().UnixNano()), nil)
+ }
Review Comment:
Returning a bus message with a `nil` payload when no snapshot is created can
be ambiguous for downstream consumers (e.g., they may still treat it as a
successful snapshot event but fail to decode/handle a nil body). Consider
returning the original `message` unchanged, or emitting an explicit typed/empty
snapshot marker that downstream can reliably interpret as 'no snapshot
created'. (Same pattern appears in other listeners in this PR.)
##
banyand/internal/storage/tsdb_test.go:
##
@@ -271,6 +272,42 @@ func TestTakeFileSnapshot(t *testing.T) {
require.NoError(t, tsdb.Close())
})
+
+ t.Run("Take snapshot with of segments", func(t *testing.T) {
Review Comment:
The subtest name has a grammatical typo; consider renaming it to clearly
express the intent (e.g., 'Take snapshot without segments').
```suggestion
t.Run("Take snapshot without segments", func(t *testing.T) {
```
--
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]
Re: [PR] ignore backup if no segments in the storage [skywalking-banyandb]
mrproliu commented on code in PR #992:
URL:
https://github.com/apache/skywalking-banyandb/pull/992#discussion_r2895918031
##
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:
Changed to return true.
--
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]
Re: [PR] ignore backup if no segments in the storage [skywalking-banyandb]
mrproliu commented on code in PR #992:
URL:
https://github.com/apache/skywalking-banyandb/pull/992#discussion_r2895915838
##
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:
Changed to added a new Info log if the segment not created.
--
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]
Re: [PR] ignore backup if no segments in the storage [skywalking-banyandb]
mrproliu commented on code in PR #992:
URL:
https://github.com/apache/skywalking-banyandb/pull/992#discussion_r2895911045
##
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:
Changed to also check the `sidxMap` length to verify the snapshot should be
created or not.
--
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]
Re: [PR] ignore backup if no segments in the storage [skywalking-banyandb]
codecov-commenter commented on PR #992: URL: https://github.com/apache/skywalking-banyandb/pull/992#issuecomment-4009888475 ## [Codecov](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/992?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report :white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 30.03%. Comparing base ([`3530dd9`](https://app.codecov.io/gh/apache/skywalking-banyandb/commit/3530dd9b5cefaa4fef60eec91ecbfbdfcaf243c5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`28848c3`](https://app.codecov.io/gh/apache/skywalking-banyandb/commit/28848c3cdbc0ac1a29fd60589904bf8af69e5717?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)). :warning: Report is 144 commits behind head on main. > :exclamation: There is a different number of reports uploaded between BASE (3530dd9) and HEAD (28848c3). Click for more details. > > HEAD has 1 upload less than BASE > >| Flag | BASE (3530dd9) | HEAD (28848c3) | >|--|--|--| >||1|0| > Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #992 +/- ## === - Coverage 45.97% 30.03% -15.95% === Files 328 122 -206 Lines 5550512846-42659 === - Hits25520 3858-21662 + Misses 27909 8615-19294 + Partials 2076 373 -1703 ``` | [Flag](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/992/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | |---|---|---| | [pkg](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/992/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `30.03% <ø> (?)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more. [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/992?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :rocket: New features to boost your workflow: - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems. - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges. -- 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]
Re: [PR] ignore backup if no segments in the storage [skywalking-banyandb]
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,
