raunaqmorarka commented on code in PR #14921: URL: https://github.com/apache/iceberg/pull/14921#discussion_r3612685701
########## core/src/main/java/org/apache/iceberg/metrics/RemoveSnapshotsReport.java: ########## @@ -0,0 +1,53 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.iceberg.metrics; + +import org.immutables.value.Value; + [email protected] +public abstract class RemoveSnapshotsReport implements MetricsReport { Review 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. ########## core/src/main/java/org/apache/iceberg/FileCleanupStrategy.java: ########## @@ -124,7 +134,92 @@ protected void deleteFiles(Set<String> pathsToDelete, String fileType) { .suppressFailureWhenFinished() .onFailure( (file, thrown) -> LOG.warn("Delete failed for {} file: {}", fileType, file, thrown)) - .run(deleteFuncToUse::accept); + .run( + file -> { + deleteFuncToUse.accept(file); + summary.deletedFile(fileType); + }); + } + } + + static class DeleteSummary { + private final AtomicLong dataFilesCount = new AtomicLong(0L); Review 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. ########## .palantir/revapi.yml: ########## @@ -1456,6 +1459,9 @@ acceptedBreaks: old: "method void org.apache.iceberg.PartitionStats::deletedEntry(org.apache.iceberg.Snapshot)" new: "method void org.apache.iceberg.PartitionStats::deletedEntry(org.apache.iceberg.Snapshot)" justification: "Changing deprecated code" + - code: "java.method.removed" + old: "method org.apache.iceberg.io.CloseableIterable<java.lang.String> org.apache.iceberg.ManifestFiles::readPaths(org.apache.iceberg.ManifestFile, org.apache.iceberg.io.FileIO)" Review Comment: No longer applies after rebasing: readPaths is kept (it was deprecated separately for 1.12.0), readColumns is added alongside it and the revapi entry is dropped. ########## core/src/main/java/org/apache/iceberg/FileCleanupStrategy.java: ########## @@ -42,6 +46,9 @@ public void accept(String file) { }; private static final Logger LOG = LoggerFactory.getLogger(FileCleanupStrategy.class); + protected static final String MANIFEST = "manifest"; + protected static final String MANIFEST_LIST = "manifest list"; + protected static final String STATISTICS_FILES = "statistics files"; Review Comment: Converted the file type strings to a DeletedFileType enum. ########## core/src/main/java/org/apache/iceberg/FileCleanupStrategy.java: ########## @@ -141,6 +236,46 @@ protected Set<String> expiredStatisticsFilesLocations( return Sets.difference(statsFileLocationsBeforeExpiration, statsFileLocationsAfterExpiration); } + protected static class FileInfo { Review Comment: Renamed to ExpiredContentFile. ########## core/src/main/java/org/apache/iceberg/FileCleanupStrategy.java: ########## @@ -124,7 +134,92 @@ protected void deleteFiles(Set<String> pathsToDelete, String fileType) { .suppressFailureWhenFinished() .onFailure( (file, thrown) -> LOG.warn("Delete failed for {} file: {}", fileType, file, thrown)) - .run(deleteFuncToUse::accept); + .run( + file -> { + deleteFuncToUse.accept(file); + summary.deletedFile(fileType); + }); + } + } + + static class DeleteSummary { + private final AtomicLong dataFilesCount = new AtomicLong(0L); + private final AtomicLong positionDeleteFilesCount = new AtomicLong(0L); + private final AtomicLong equalityDeleteFilesCount = new AtomicLong(0L); + private final AtomicLong manifestsCount = new AtomicLong(0L); + private final AtomicLong manifestListsCount = new AtomicLong(0L); + private final AtomicLong statisticsFilesCount = new AtomicLong(0L); + + public void deletedFiles(String type, int numFiles) { + if (FileContent.DATA.name().equalsIgnoreCase(type)) { + dataFilesCount.addAndGet(numFiles); + + } else if (FileContent.POSITION_DELETES.name().equalsIgnoreCase(type)) { + positionDeleteFilesCount.addAndGet(numFiles); + + } else if (FileContent.EQUALITY_DELETES.name().equalsIgnoreCase(type)) { + equalityDeleteFilesCount.addAndGet(numFiles); + + } else if (MANIFEST.equalsIgnoreCase(type)) { + manifestsCount.addAndGet(numFiles); + + } else if (MANIFEST_LIST.equalsIgnoreCase(type)) { + manifestListsCount.addAndGet(numFiles); + + } else if (STATISTICS_FILES.equalsIgnoreCase(type)) { + statisticsFilesCount.addAndGet(numFiles); + + } else { + throw new ValidationException("Illegal file type: %s", type); + } + } + + public void deletedFile(String type) { Review Comment: Done, deletedFile now delegates to deletedFiles(type, 1) and the summary counters are keyed by the file type enum. ########## core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java: ########## @@ -259,32 +261,44 @@ public void cleanFiles( }); if (ExpireSnapshots.CleanupLevel.ALL == cleanupLevel) { - Set<String> filesToDelete = + Set<FileInfo> filesToDelete = findFilesToDelete( manifestsToScan, manifestsToRevert, validIds, beforeExpiration.specsById()); - LOG.debug("Deleting {} data files", filesToDelete.size()); - deleteFiles(filesToDelete, "data"); + 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()) { Review 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. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
