openinx commented on code in PR #18463:
URL: https://github.com/apache/iceberg/pull/18463#discussion_r4533937779
##########
gcp/src/main/java/org/apache/iceberg/gcp/gcs/GCSFileIO.java:
##########
@@ -327,16 +331,56 @@ public void deleteFiles(Iterable<String> pathsToDelete)
throws BulkDeletionFailu
@SuppressWarnings("resource")
private void internalDeleteFiles(Stream<BlobId> blobIdsToDelete) {
- Streams.stream(
- Iterators.partition(
- blobIdsToDelete.iterator(),
-
clientForStoragePath(ROOT_STORAGE_PREFIX).gcpProperties().deleteBatchSize()))
- .forEach(
- batch -> {
- if (!batch.isEmpty()) {
-
clientForStoragePath(batch.get(0).toGsUtilUri()).storage().delete(batch);
- }
- });
+ List<String> failedDeletions =
+ Streams.stream(
+ Iterators.partition(
+ blobIdsToDelete.iterator(),
+
clientForStoragePath(ROOT_STORAGE_PREFIX).gcpProperties().deleteBatchSize()))
+ .filter(batch -> !batch.isEmpty())
+ .flatMap(
+ batch ->
+
deleteBatch(clientForStoragePath(batch.get(0).toGsUtilUri()).storage(), batch)
+ .stream())
+ .toList();
+
+ if (!failedDeletions.isEmpty()) {
+ throw new BulkDeletionFailureException(failedDeletions.size());
+ }
+ }
+
+ /**
+ * Deletes a batch of objects and returns the URIs that actually failed to
delete.
+ *
+ * <p>GCS reports a missing object as {@code false} (HTTP 404) rather than
an error, so we let
+ * those pass: a retried cleanup often re-deletes files a previous attempt
already removed. Only a
+ * {@link StorageException} such as HTTP 403 (access denied) is a genuine
failure. If the batch
+ * request itself fails, every object in it is reported as failed.
+ *
+ * @return the URIs of the objects that failed to delete, empty if all
succeeded
+ */
+ private static List<String> deleteBatch(Storage storage, List<BlobId>
blobIds) {
+ StorageBatch batch = storage.batch();
+ Map<BlobId, StorageBatchResult<Boolean>> results = new LinkedHashMap<>();
+ blobIds.forEach(blobId -> results.put(blobId, batch.delete(blobId)));
+
+ try {
+ batch.submit();
+ } catch (StorageException e) {
Review Comment:
**[P2] Stop cleanup when the task is canceled**
With Apache HTTP transport, canceling a delete while it waits for a
connection can throw `StorageException`. This catch treats cancellation like a
normal delete failure and keeps trying the remaining batches. Can we let
cancellation errors propagate so cleanup stops?
--
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]
