raunaqmorarka commented on code in PR #14921:
URL: https://github.com/apache/iceberg/pull/14921#discussion_r3612715382
##########
api/src/main/java/org/apache/iceberg/ExpireSnapshots.java:
##########
@@ -161,4 +162,10 @@ default ExpireSnapshots cleanExpiredMetadata(boolean
clean) {
throw new UnsupportedOperationException(
this.getClass().getName() + " doesn't implement cleanExpiredMetadata");
}
+
+ /** Report metrics about the ExpireSnapshots operation to the provided
reporter */
+ default ExpireSnapshots metricsReporter(MetricsReporter reporter) {
Review Comment:
Moved the setter to RemoveSnapshots as a package private reportWith,
following the SnapshotProducer approach, and BaseTable now wires the table's
reporter into it. The ExpireSnapshots interface is unchanged.
##########
.palantir/revapi.yml:
##########
@@ -1369,6 +1369,9 @@ acceptedBreaks:
old: "class org.apache.iceberg.encryption.EncryptingFileIO"
new: "class org.apache.iceberg.encryption.EncryptingFileIO"
justification: "New method for Manifest List reading"
+ - code: "java.method.addedToInterface"
+ new: "method org.apache.iceberg.ExpireSnapshots
org.apache.iceberg.ExpireSnapshots::metricsReporter(org.apache.iceberg.metrics.MetricsReporter)"
Review Comment:
Resolved by moving the setter to RemoveSnapshots, the revapi entry is
dropped.
##########
core/src/main/java/org/apache/iceberg/ReachableFileCleanup.java:
##########
@@ -208,8 +226,12 @@ private Set<String> findFilesToDelete(
}
// Remove all the live files from the candidate deletion set
- try (CloseableIterable<String> paths =
ManifestFiles.readPaths(manifest, fileIO)) {
- paths.forEach(filesToDelete::remove);
+ try (CloseableIterable<DataFile> entries =
+ ManifestFiles.readColumns(
+ manifest, fileIO, ImmutableList.of("content",
"file_path"))) {
Review Comment:
Done, it's now a DELETE_CANDIDATE_COLUMNS constant.
##########
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 {
+ private final FileContent content;
+ private final String path;
+
+ public FileInfo(FileContent content, String path) {
+ this.content = content;
+ this.path = path;
Review Comment:
Done, both fields are checked with Preconditions.checkNotNull now.
##########
core/src/main/java/org/apache/iceberg/IncrementalFileCleanup.java:
##########
@@ -320,12 +336,14 @@ private Set<String> findFilesToDelete(
.run(
manifest -> {
// the manifest has deletes, scan it to find files to delete
- try (ManifestReader<?> reader = ManifestFiles.open(manifest,
fileIO, specsById)) {
- for (ManifestEntry<?> entry : reader.entries()) {
+ try (ManifestReader<? extends ContentFile<?>> reader =
+ ManifestFiles.open(manifest, fileIO, specsById)) {
+ for (ManifestEntry<? extends ContentFile<?>> entry :
reader.entries()) {
// delete any ADDED file from manifests that were reverted
if (entry.status() == ManifestEntry.Status.ADDED) {
// use toString to ensure the path will not change (Utf8
is reused)
Review Comment:
Removed both occurrences.
--
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]
