feat(snapshots): Add periodic JSON progress output to snapshot verify #4645
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4645 +/- ##
==========================================
+ Coverage 75.86% 76.44% +0.57%
==========================================
Files 470 530 +60
Lines 37301 40363 +3062
==========================================
+ Hits 28299 30856 +2557
- Misses 7071 7476 +405
- Partials 1931 2031 +100 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
Adds periodic JSON progress output to the snapshot verifier, enhances stats tracking, and updates CLI commands and tests to use the new VerifierResult pattern.
- Extended
Verifierto track bytes, files, and expected totals, with JSON and periodic logging. - Refactored
InParallelto returnVerifierResultincluding stats and error lists. - Updated CLI (
snapshot verify) to support--jsonoutput and to seed expected totals from directory summaries. - Modified tests to assert on the new result structure and JSON fields.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| snapshot/snapshotfs/snapshot_verifier.go | Added mutex-protected stats, JSONStats flag, VerifierStats struct, JSON output, and updated InParallel. |
| snapshot/snapshotfs/snapshot_verifier_test.go | Updated tests to unpack (VerifierResult, error) and assert on stats and errors. |
| snapshot/snapshotfs/snapshot_tree_walker.go | Added GetErrors() to expose accumulated errors and counts. |
| cli/command_snapshot_verify.go | Introduced --json flag, JSON output handling, and expected-totals seeding. |
| cli/command_snapshot_verify_test.go | New tests verifying JSON output of snapshot verify. |
| cli/command_snapshot_fix_invalid_files.go | Adjusted VerifyFile calls to the new signature including file size. |
Comments suppressed due to low confidence (2)
snapshot/snapshotfs/snapshot_verifier.go:130
- [nitpick] The JSON tag for
ExpectedTotalFilesisexpectedTotalFileCount(singular "File"), butExpectedTotalBytesusesexpectedTotalBytes(plural); consider unifying the naming convention for consistency across stats fields.
ExpectedTotalFiles int64 `json:"expectedTotalFileCount"`
cli/command_snapshot_verify_test.go:87
- Tests validate processed counts and errors but do not assert on the new expected-totals (ExpectedTotalObjects, ExpectedTotalFiles, ExpectedTotalBytes). Consider adding assertions to cover these fields in the JSON output.
result := unmarshalSnapVerify(t, stdout[0])
| statsMu sync.RWMutex | ||
|
|
||
| // +checklocks:statsMu | ||
| queued int32 |
There was a problem hiding this comment.
The field queued is incremented but never used in any stats output or logic; consider removing it or exposing it in the JSON stats if it's needed for progress reporting.
There was a problem hiding this comment.
@redgoat650 as a followup, either:
- include
queuedin the (JSON) stats; or - remove
queuedaltogether, as it is only written to, not read.
The former may make sense now that the JSON stats are reported.
- Add read stats to snapshot verifier output - Add periodic JSON progress output. - Refactor the use of directory summary. - Use stats mutex for all stats. - Add processedBytes to the snapshot verify output - Output more frequently, when bytes processed changes
06a9099 to
a9ece98
Compare
| // addExpectedWorkFromDirSummaryToVerifier initializes the snapshot verifier with an | ||
| // expected amount of work that will take place during the tree walk for this Entry. | ||
| // If the entry is not a DirectoryWithSummary, or the Summary returns nil, no stats | ||
| // will be added to the totals. |
There was a problem hiding this comment.
@redgoat650
Concern: this may silently distort the expected numbers, and thus the expectations.
Also, it may create confusion when displaying (JSON) results where the verified number of files / bytes is greater than the expected values.
| statsMu sync.RWMutex | ||
|
|
||
| // +checklocks:statsMu | ||
| queued int32 |
There was a problem hiding this comment.
@redgoat650 as a followup, either:
- include
queuedin the (JSON) stats; or - remove
queuedaltogether, as it is only written to, not read.
The former may make sense now that the JSON stats are reported.
Add read stats to snapshot verifier output.
Add periodic JSON progress output.
Refactor the use of directory summary.
Use stats mutex for all stats.
Add processedBytes to the snapshot verify output.
Output more frequently, when bytes processed changes.
Change depends on feat(snapshots): Add JSON output flag for snapshot verify #4644; full diff includes both changes until that PR merges. Please see commit c59be0f for changes introduced by this PR.