You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "mrproliu (via GitHub)" <gi...@apache.org> on 2026/05/15 12:58:38 UTC
[PR] Fix incorrect counts, rate and missing trace fields in the lifecycle migration report [skywalking-banyandb]
mrproliu opened a new pull request, #1133:
URL: https://github.com/apache/skywalking-banyandb/pull/1133
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Fixes apache/skywalking#<issue number>.
- [x] Update the [`CHANGES` log](https://github.com/apache/skywalking-banyandb/blob/main/CHANGES.md).
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix incorrect counts, rate and missing trace fields in the lifecycle migration report [skywalking-banyandb]
Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily commented on PR #1133:
URL: https://github.com/apache/skywalking-banyandb/pull/1133#issuecomment-4465182051
The v2 report contract must ensure one invariant:
**`completion_rate == 100%` means "the migration finished successfully" for every resource type — parts, series, and element_index alike. Nothing less, and never more.**
Option 1 and option 2 both leave the primary `completion_rate` able to read 266 % or 5 %, which violates the invariant regardless of documentation or sibling fields.
Option 3 is the path that satisfies it. Reshape the visitor so `Counts` reflects source-cardinality and the numerator counts source items whose entire fan-out succeeded. Per-target tracking for resume granularity can stay internal — the report only needs to expose source-level numbers.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix incorrect counts, rate and missing trace fields in the lifecycle migration report [skywalking-banyandb]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1133:
URL: https://github.com/apache/skywalking-banyandb/pull/1133#discussion_r3248354087
##########
banyand/backup/lifecycle/service.go:
##########
@@ -538,7 +547,10 @@ func (l *lifecycleService) buildMigrationReport(p *Progress) map[string]interfac
// buildSummaryStats creates overall migration statistics.
func (l *lifecycleService) buildSummaryStats(p *Progress) map[string]interface{} {
- totalGroups := len(p.CompletedGroups) + len(p.DeletedStreamGroups) + len(p.DeletedMeasureGroups)
+ // total_groups reflects the cycle's scheduled set captured at the top of
+ // action(); completed_groups counts groups that ran the full
+ // processXxxGroup → MarkGroupCompleted path.
+ totalGroups := len(p.GroupsToProcess)
completedGroups := len(p.CompletedGroups)
Review Comment:
buildSummaryStats() now uses GroupsToProcess as the denominator for migration_status.total_groups, but still uses len(p.CompletedGroups) for completed_groups. On resume/retry, CompletedGroups may include groups that are not in the current GroupsToProcess set (because getGroupsToProcess filters completed groups), which can make completed_groups > total_groups and completion_rate > 100. Consider counting only completed groups within GroupsToProcess (e.g., iterate p.GroupsToProcess and count p.CompletedGroups[name]).
##########
banyand/backup/lifecycle/service.go:
##########
@@ -436,6 +436,10 @@ func (l *lifecycleService) action(ctx context.Context) error {
Str("measure_snapshot", measureDir).
Str("trace_snapshot", traceDir).
Msg("created snapshots")
+ // Record the scheduled group set only after snapshots are confirmed so
+ // the report distinguishes "no work this cycle" (total_groups=0) from a
+ // real cycle that processed N groups (total_groups=N).
+ progress.SetGroupsToProcess(getGroupNames(groups))
progress.Save(l.progressFilePath, l.l)
Review Comment:
GroupsToProcess is persisted and later used as the denominator for migration_status.total_groups. Since it is only set after snapshots are created, any early-return paths before this point (e.g., when no snapshots are found and a report is still generated) can emit a report with a stale GroupsToProcess from a previous run. Consider explicitly resetting GroupsToProcess to empty for 'no work this cycle' paths, or setting it earlier and clearing it when snapshots are empty, so total_groups is accurate.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix incorrect counts, rate and missing trace fields in the lifecycle migration report [skywalking-banyandb]
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu merged PR #1133:
URL: https://github.com/apache/skywalking-banyandb/pull/1133
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix incorrect counts, rate and missing trace fields in the lifecycle migration report [skywalking-banyandb]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1133:
URL: https://github.com/apache/skywalking-banyandb/pull/1133#issuecomment-4460039006
## [Codecov](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1133?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 39.59%. 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 ([`37bd4f6`](https://app.codecov.io/gh/apache/skywalking-banyandb/commit/37bd4f6fb1440352c2536a8a11a8f8eb6cc9d9b6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 255 commits behind head on main.
> :exclamation: There is a different number of reports uploaded between BASE (3530dd9) and HEAD (37bd4f6). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (3530dd9) | HEAD (37bd4f6) |
>|------|------|------|
>||1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #1133 +/- ##
==========================================
- Coverage 45.97% 39.59% -6.39%
==========================================
Files 328 180 -148
Lines 55505 17370 -38135
==========================================
- Hits 25520 6877 -18643
+ Misses 27909 9863 -18046
+ Partials 2076 630 -1446
```
| [Flag](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1133/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/1133/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.59% <ø> (?)` | |
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.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/skywalking-banyandb/pull/1133?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).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :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.
</details>
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix incorrect counts, rate and missing trace fields in the lifecycle migration report [skywalking-banyandb]
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu commented on PR #1133:
URL: https://github.com/apache/skywalking-banyandb/pull/1133#issuecomment-4465122830
Quick clarification on the trade-off before I pick a direction:
Just restoring pre-walk and reverting Mark*++Counts doesn't fully bring the planned semantic back — for series and element_index, the visitor sets
Counts = len(segmentFiles)
at the top of VisitSeries / VisitElementIndex, but Mark* runs inside for targetSegments × shardIDs. When fan-out > 1 (typical hot → warm), Mark calls exceed source files and the report ends up with e.g. series.completion_rate = 266.67%,
while element_index flips the other way to single-digit % with errors = 0. Operators in the original SRE report flagged exactly those numbers.
So "Counts = pre-walk source" and "rate stays in [0,100]" cannot both hold for series / element_index under the current visitor shape. To avoid the 266% regression there are three options:
1. Document & accept: keep pre-walk semantic, treat rate > 100% as "fan-out write count exceeded source count" and document it.
2. Bump report_version to 2.1: add planned_* fields from pre-walk alongside existing fields; old fields stay [0,100], planned_completion_rate gives the mid-flight progress signal.
3. Refactor the visitor: collapse per-source Mark* to one call (lose per-target granularity for retries, but Counts = source = mark calls becomes consistent).
Which one do you prefer?
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org