aokolnychyi commented on code in PR #5459:
URL: https://github.com/apache/iceberg/pull/5459#discussion_r956230854
##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java:
##########
@@ -251,6 +253,29 @@ public void testDeleteTableWithoutPurge() {
public void testDeleteTableWithPurge() {
String namespace = createNamespace();
String tableName = createTable(namespace);
+ Table table = glueCatalog.loadTable(TableIdentifier.of(namespace,
tableName));
+
+ final DataFile testFile =
Review Comment:
nit: We don't really use `final` with temp vars in Iceberg.
I know it is debatable but we've agreed it is not worth is.
##########
core/src/main/java/org/apache/iceberg/CatalogUtil.java:
##########
@@ -102,37 +106,14 @@ public static void dropTableData(FileIO io, TableMetadata
metadata) {
deleteFiles(io, manifestsToDelete);
}
- Tasks.foreach(Iterables.transform(manifestsToDelete, ManifestFile::path))
- .executeWith(ThreadPools.getWorkerPool())
- .noRetry()
- .suppressFailureWhenFinished()
- .onFailure((manifest, exc) -> LOG.warn("Delete failed for manifest:
{}", manifest, exc))
- .run(io::deleteFile);
-
- Tasks.foreach(manifestListsToDelete)
- .executeWith(ThreadPools.getWorkerPool())
- .noRetry()
- .suppressFailureWhenFinished()
- .onFailure((list, exc) -> LOG.warn("Delete failed for manifest list:
{}", list, exc))
- .run(io::deleteFile);
-
- Tasks.foreach(
- Iterables.transform(metadata.previousFiles(),
TableMetadata.MetadataLogEntry::file))
- .executeWith(ThreadPools.getWorkerPool())
- .noRetry()
- .suppressFailureWhenFinished()
- .onFailure(
- (metadataFile, exc) ->
- LOG.warn("Delete failed for previous metadata file: {}",
metadataFile, exc))
- .run(io::deleteFile);
-
- Tasks.foreach(metadata.metadataFileLocation())
- .noRetry()
- .suppressFailureWhenFinished()
- .onFailure(
- (metadataFile, exc) ->
- LOG.warn("Delete failed for metadata file: {}", metadataFile,
exc))
- .run(io::deleteFile);
+ deleteFiles(io, Iterables.transform(manifestsToDelete,
ManifestFile::path), "manifest", true);
+ deleteFiles(io, manifestListsToDelete, "manifest list", true);
+ deleteFiles(
+ io,
+ Iterables.transform(metadata.previousFiles(),
TableMetadata.MetadataLogEntry::file),
+ "previous metadata",
+ true);
+ deleteFiles(io, ImmutableList.of(metadata.metadataFileLocation()),
"metadata", true);
Review Comment:
nit: Just `deleteFile` as we have only one file?
##########
core/src/main/java/org/apache/iceberg/CatalogUtil.java:
##########
@@ -151,27 +132,69 @@ private static void deleteFiles(FileIO io,
Set<ManifestFile> allManifests) {
.run(
manifest -> {
try (ManifestReader<?> reader = ManifestFiles.open(manifest,
io)) {
+ List<String> pathsToDelete = Lists.newArrayList();
for (ManifestEntry<?> entry : reader.entries()) {
// intern the file path because the weak key map uses
identity (==) instead of
// equals
String path = entry.file().path().toString().intern();
Boolean alreadyDeleted = deletedFiles.putIfAbsent(path,
true);
if (alreadyDeleted == null || !alreadyDeleted) {
- try {
- io.deleteFile(path);
- } catch (RuntimeException e) {
- // this may happen if the map of deleted files gets
cleaned up by gc
- LOG.warn("Delete failed for data file: {}", path, e);
- }
+ pathsToDelete.add(path);
}
}
+
+ String type = reader.isDeleteManifestReader() ? "delete" :
"data";
+ deleteFiles(io, pathsToDelete, type, false);
} catch (IOException e) {
throw new RuntimeIOException(
e, "Failed to read manifest file: %s", manifest.path());
}
});
}
+ /**
+ * Helper to delete files. Bulk deletion is used if possible.
+ *
+ * @param io FileIO for deletes
+ * @param files files to delete
+ * @param type type of files being deleted
+ * @param concurrent controls concurrent deletion. Only applicable for
non-bulk FileIO
+ */
+ private static void deleteFiles(
+ FileIO io, Iterable<String> files, String type, boolean concurrent) {
+ if (io instanceof SupportsBulkOperations) {
+ try {
+ SupportsBulkOperations bulkIo = (SupportsBulkOperations) io;
Review Comment:
nit: `bulkIo` -> `bulkIO`?
--
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]
