Core: Report metrics about deleted files in ExpireSnapshots - #14921
Core: Report metrics about deleted files in ExpireSnapshots#14921raunaqmorarka wants to merge 1 commit into
Conversation
81eea28 to
da13abd
Compare
| import org.immutables.value.Value; | ||
|
|
||
| @Value.Immutable | ||
| public abstract class RemoveSnapshotsReport implements MetricsReport { |
There was a problem hiding this comment.
i wonder if there is a way to get this according to the contract defined here :
There was a problem hiding this comment.
I wanted to use MetricsReporter as that is a more flexible approach and similar to org.apache.iceberg.Scan#metricsReporter
There was a problem hiding this comment.
I'm a bit skeptical on introducing a new report type. Don't we already have all of this information in the CommitReport, which carries a CommitMetricsResult ?
There was a problem hiding this comment.
No report is emitted for expiration today: CommitReport is only built in SnapshotProducer, and RemoveSnapshots is not a SnapshotProducer. CommitMetricsResult is also derived from a single commit's SnapshotSummary (logical removals in that commit), while this report counts physical deletions across all expired snapshots after reachability checks, minus delete failures, plus manifest list and statistics file deletions that no existing counter covers, so a per-operation report type follows the ScanReport/CommitReport pattern.
da13abd to
e6c8161
Compare
37c631d to
c21feaf
Compare
| Map<FileContent, Set<String>> groupedFilesToDelete = | ||
| filesToDelete.stream() | ||
| .collect( | ||
| Collectors.groupingBy( | ||
| FileInfo::getContent, | ||
| Collectors.mapping(FileInfo::getPath, Collectors.toSet()))); | ||
|
|
||
| for (Map.Entry<FileContent, Set<String>> entry : groupedFilesToDelete.entrySet()) { |
There was a problem hiding this comment.
Why group now? It seems orthogonal to collecting summary
There was a problem hiding this comment.
Grouping by content type is what attributes bulk deletion counts to the right counter in the summary, the bulk delete path only gets a single file type per call.
| } | ||
|
|
||
| static class DeleteSummary { | ||
| private final AtomicLong dataFilesCount = new AtomicLong(0L); |
There was a problem hiding this comment.
don't we already have all of this in the SnapshotSummary when the snapshot is committed?
There was a problem hiding this comment.
Expiration produces no snapshot, so there is no SnapshotSummary for it, and summary counts are logical removals of one commit rather than files physically deleted after checking reachability from all retained snapshots, branches and tags. The concrete consumer is engines calling this API directly (e.g. the expire_snapshots procedure in Trino), which currently have no way to report what was cleaned up.
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
465bfee to
331f815
Compare
3101df5 to
f12f421
Compare
raunaqmorarka
changed the title
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
f12f421 to
41cd18a
Compare
4 participants
ExpireSnapshotsdoes not expose any information about what it actually deleted. This adds aRemoveSnapshotsReportcarrying the number of deleted data files, position deletes, equality deletes, manifests, manifest lists and statistics files, reported through the existingMetricsReportermechanism.The counts reflect files that were physically deleted, so bulk deletion failures are excluded. They cannot be derived from existing reports: expiration produces no snapshot, so there is no
SnapshotSummaryand noCommitReportfor it, andCommitMetricsResultdescribes logical removals within a single commit rather than files deleted after checking reachability from all retained snapshots, branches and tags. It also has no counters for manifest lists or statistics files.Reporting follows the
SnapshotProducerpattern rather than adding to theExpireSnapshotsinterface.RemoveSnapshotsgains a package privatereportWith(MetricsReporter), andBaseTable.expireSnapshots()andBaseTransaction.expireSnapshots()pass in the table's reporter. Callers holding aBaseTablecan attach their own reporter withcombineMetricsReporterbefore callingexpireSnapshots().Since the table always has a reporter, expiration now always emits a report, where previously nothing was emitted. With the default
LoggingMetricsReporterthis is one additional log line per expiration.ReachableFileCleanupnow reads thecontentcolumn alongsidefile_pathso deleted files can be attributed to the right counter, which is what the newManifestFiles.readColumnsis for.This lets engines report expiration results to users, for example the
expire_snapshotsprocedure in Trino, which currently has no way to show output comparable to the Spark procedure without duplicating the reachability analysis.