AWS: Handle S3 Table Bucket purge gracefully in GlueCatalog (#14449) by yadavay-amzn · Pull Request #16073 · apache/iceberg

yadavay-amzn · 2026-04-21T20:58:12Z

When calling GlueCatalog.dropTable() with purge=true on a table in an S3 Table Bucket, the purge fails because S3 Table Buckets do not allow manual file deletion.

This change wraps CatalogUtil.dropTableData() in a try-catch so that purge failures are logged as warnings instead of propagating and failing the entire drop operation. The table is still successfully dropped from the Glue catalog.

Closes #14449

…4449) When calling GlueCatalog.dropTable() with purge=true on a table in an S3 Table Bucket, the purge fails because S3 Table Buckets do not allow manual file deletion. This change wraps CatalogUtil.dropTableData() in a try-catch so that purge failures are logged as warnings instead of propagating and failing the entire drop operation. Closes apache#14449

ebyhr · 2026-04-21T22:32:35Z

-        LOG.info("Glue table {} data purged", identifier);
+        try {
+          CatalogUtil.dropTableData(ops.io(), lastMetadata);
+          LOG.info("Glue table {} data purged", identifier);


Is there any way to check whether the target table exists in S3 Table bucket?

The location could be checked for S3 Table Bucket ARN patterns, but catching the exception is more robust as it handles any case where purge fails (permissions, bucket policies, etc.) without needing to enumerate all possible URI formats.
Looks like this also aligns with the Trino approach you linked!

Happy to add a URI check if you'd prefer a more targeted approach though.

I was wondering if we could do both (S3 Table check + try-catch) to avoid redundant S3 requests and warning logs. I think we should keep the try-catch regardless of S3 Table because it may fail for other reasons.

The "Enumerate all possible URI formats" approach doesn't look straightforward. Only adding try-catch looks good to me.

ebyhr · 2026-04-21T22:38:47Z

+          LOG.info("Glue table {} data purged", identifier);
+        } catch (Exception e) {
+          LOG.warn(
+              "Failed to purge data for table: {}, continuing drop without purge", identifier, e);


The table has already been dropped by the time we reach this line, so this change makes sense to me.

The Trino Iceberg connector also suppresses failures when it cannot delete data using the Glue catalog:

https://github.com/trinodb/trino/blob/5a116341b53f9f3a3b29b8b405773010e307e40b/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java#L676-L696

singhpk234 · 2026-04-23T07:02:56Z

+        try {
+          CatalogUtil.dropTableData(ops.io(), lastMetadata);
+          LOG.info("Glue table {} data purged", identifier);
+        } catch (Exception e) {


is it possible to catch the Specific exception rather than catch all Exception ?

Intentionally catching broad Exception here — the table metadata has already been dropped by this point, so the try-catch is a safety net to prevent any unexpected failure from blocking the drop operation. Narrowing to a specific exception risks missing edge cases from different IO implementations (S3, GCS, HDFS, etc.). This also aligns with the Trino approach that @ebyhr linked.

I'm hesitant against this broad of an exception. We should be strict in what we are catching here it's a purge that other use cases could hit.

Fair point — two reviewers have raised this now. What exception type would you prefer here? The challenge is that CatalogUtil.dropTableData delegates to different IO implementations (S3, GCS, HDFS) which throw different exception types. Would RuntimeException be narrow enough, or would you prefer something S3-specific like SdkServiceException?

geruh

Stepping back here, this PR is the first time in OSS (on the Java side) that GlueCatalog acknowledges the "federation" story from Glue. Where Glue can talk to linked catalogs, whether they're IRC backed or, in this case, S3 Tables. And in some cases federation is painful, like this managed storage story where S3 Tables limits normal S3 operations, and uses work arounds for location allocation.

With that said, it's worth being explicit about that rather than implicit with a broad try/catch.

So the root cause is that this Glue Table is federated from an S3 Tables catalog where data is managed server side, so our client side CatalogUtil.dropTableData is unnecessary and will fail. Catching Exception around dropTableData can swallow unrelated failures like iam issues or bugs we otherwise would want surfaced in non s3table catalogs especially during a purge.

There are 3 options here:

Build support for Glues federation use case. Glue responses expose a federated connection type. So in this case if its s3 tables we could skip client side purge. This is what we do today.
Decide GlueCatalog doesn't model federation yet and keep this scoped down. If we're not ready to introduce the glue federation support. Then this PR at a minimum narrow this try/catch to what S3Tables throws on delete and log. Furthermore, we need some documentation on this.
Block all requests against federated tables. This is quite difficult if I remember correctly because a federated table can be linked to a database? I'd need to think this one through more.

I'd perfer option 1 because it will scale with the federation stories in Glue.

WDYT?

geruh · 2026-04-23T22:28:05Z

+        try {
+          CatalogUtil.dropTableData(ops.io(), lastMetadata);
+          LOG.info("Glue table {} data purged", identifier);
+        } catch (Exception e) {


I'm hesitant against this broad of an exception. We should be strict in what we are catching here it's a purge that other use cases could hit.

geruh · 2026-04-23T22:31:49Z

              .build());
      LOG.info("Successfully dropped table {} from Glue", identifier);
      if (purge && lastMetadata != null) {
-        CatalogUtil.dropTableData(ops.io(), lastMetadata);


Technically, s3tables is purging data behind the scenes right. So in the case of dropTable(identifier, false) we should throw and force the user to be specific about purging.

Good point about S3 Tables purging behind the scenes. The current fix only affects the purge=true path — when purge=false, dropTableData is never called so there's no change in behavior. But I agree the broader S3 Tables story may need more thought beyond this PR.

This is a matter of expected behavior someone could call drop table with purge set to false, and their data will still be deleted by S3Tables. I'm leaning towards forcing the user to specify purge when dropping otherwise fail.

geruh · 2026-04-23T22:32:41Z

  }

+  @Test
+  public void testDropTableWithPurgeFailure() {


Were you able to test this functionality against a real s3tables catalog?

Haven't tested against a real S3 Tables catalog. Is that strictly necessary for this change, or would the unit test coverage be sufficient?

Well we'd know what the actual exceptions to account for in catch above with some real testing right. We should be strict in handling here to avoid masking any real bugs.

github-actions · 2026-05-26T00:43:50Z

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.

github-actions Bot added the AWS label Apr 21, 2026

yadavay-amzn force-pushed the fix/14449-glue-s3-table-purge branch from 840236c to e996c80 Compare April 21, 2026 22:02

ebyhr reviewed Apr 21, 2026

View reviewed changes

singhpk234 reviewed Apr 23, 2026

View reviewed changes

geruh reviewed Apr 23, 2026

View reviewed changes

github-actions Bot added the stale label May 26, 2026

Conversation

yadavay-amzn commented Apr 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geruh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

geruh left a comment •

edited

Loading