feat(cleanup): add more metrics to RemovalStats#6025
feat(cleanup): add more metrics to RemovalStats#6025majin1102 merged 6 commits intolance-format:mainfrom
Conversation
majin1102
left a comment
There was a problem hiding this comment.
Thanks for working on this. Left one comment
| pub struct CleanupStats { | ||
| pub bytes_removed: u64, | ||
| pub old_versions: u64, | ||
| pub removed_data_file_num: u64, |
There was a problem hiding this comment.
This is a good start.
Wondering if we could include transaction files, index files, deletion files, etc. And I think removed_data_files might be better. We have similar fields in ManifestSummary. data_files_removed might be another option
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Hi @majin1102 Thanks a lot for your review. All comments are addressed. PTAL~ |
Xuanwo
left a comment
There was a problem hiding this comment.
Thank you for working on this! I only have a suggestion on naming, other LGTM.
| pub removed_data_files: u64, | ||
| pub removed_transaction_files: u64, | ||
| pub removed_index_files: u64, | ||
| pub removed_deletion_files: u64, |
There was a problem hiding this comment.
| pub removed_data_files: u64, | |
| pub removed_transaction_files: u64, | |
| pub removed_index_files: u64, | |
| pub removed_deletion_files: u64, | |
| pub data_files_removed: u64, | |
| pub transaction_files_removed: u64, | |
| pub index_files_removed: u64, | |
| pub deletion_files_removed: u64, |
I suggest we keep the same naing style along with existing bytes_removed.
Xuanwo
left a comment
There was a problem hiding this comment.
Thank you for working on this, LGTM. I'm fine once all CI passed.
Add a new metric
removed_data_file_numto theRemovalStatsof the cleanup operation results,So that users can easily perceive how many lance data files have been deleted in the current cleanup operation and better evaluate the impact scope of the current cleanup.