Fix query-scope cache invalidation for Glue catalog by piotrrzysko · Pull Request #28266 · trinodb/trino

piotrrzysko · 2026-02-12T09:00:31Z

Description

This PR fixes a bug where GlueIcebergTableOperations#commitTableUpdate was not invalidating the query-scope table metadata cache after a successful commit. As a result, Iceberg observed stale metadata when determining committed files. This caused uncommitted manifest files from failed retry attempts to remain as orphans instead of being cleaned up.

Automatic cleanup is performed in BaseTransaction#commitSimpleTransaction However, it was never triggered because committedFiles was null due to stale information returned by TableOperations::current.

The fix refactors the code to wrap the Glue client in a caching client implementation, centralizing cache management and preventing similar bugs in the future.

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:

## Section
* Fix orphaned manifest files not being cleaned up after commit retries when using the Glue catalog. ({issue}`issuenumber`)

findepi

"Add test for uncommitted manifest cleanup"

findepi · 2026-02-12T13:50:09Z

...t/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergPolarisCatalogConnectorSmokeTest.java

+        //TODO: Fix https://github.com/trinodb/trino/issues/23941
+        abort("Skipped for now due to #23941");


the test is self-maintained when it follows the pattern

assertThatThrownBy(super::test_name) .hasMessage...(...);

I’m not sure yet about this. I think it fails nondeterministically, and I couldn’t find the error message with which it failed for me. Locally, I couldn’t reproduce failures for either this test or the one above.

you're right. this pattern is not applicable for tests which fail nondeterministically

...rc/test/java/io/trino/plugin/iceberg/catalog/rest/TestIcebergS3TablesConnectorSmokeTest.java

...java/io/trino/plugin/iceberg/catalog/rest/TestIcebergUnityRestCatalogConnectorSmokeTest.java

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorSmokeTest.java

findepi · 2026-02-12T13:55:35Z

...n/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMotoConnectorSmokeTest.java

    public void testDeleteRowsConcurrently() {}

+    @Test
+    @Disabled("Moto is not concurrency safe")


😮

Does it make sense to continue to test with Moto? What value does it add?

Does it make sense to continue to test with Moto?

That’s a good question, but I think it's outside the scope of this PR. I followed the existing pattern (see the test above).

That’s a good question, but I think it's outside the scope of this PR.

of course!

findepi · 2026-02-12T14:00:23Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java

@@ -449,7 +443,7 @@ private void getColumnsFromIcebergMetadata(
        for (SchemaTableName tableName : relationFilter.apply(glueTables.keySet())) {
            Table table = glueTables.get(tableName);
            // potentially racy with invalidation, but TrinoGlueCatalog is session-scoped


This comment was already here. Now we're adding QueryScopedCachingTrinoGlueClient.
Do we really need two levels of caching & invalidation?

By two levels of caching & invalidation do you mean TrinoGlueCatalog (level 1) and QueryScopedCachingTrinoGlueClient (level 2)?

If so, I introduced QueryScopedCachingTrinoGlueClient to keep cache invalidation close to the state-modifying operations. Previously, they were decoupled, which in my opinion led to the bug I’m trying to fix.

I get there is a subtle bug manifesting in partial cleanup.

Is the bug fixiable without introducing QueryScopedCachingTrinoGlueClient? what would the fix look like, and what would be the cons of doing it?

I’m fixing a missing invalidation in GlueIcebergTableOperations#commitTableUpdate, where the raw GlueClient was used. I introduced QueryScopedCachingTrinoGlueClient to share invalidation logic between TrinoGlueCatalog and GlueIcebergTableOperations.

I didn’t intend to address this pre-existing code comment.

Verify that uncommitted manifest files are properly cleaned up. The test uses concurrent INSERT operations to create conflicts, then checks if any orphaned manifests remain.

GlueIcebergTableOperations#commitTableUpdate was not invalidating the query-scope table metadata cache after a successful commit. This caused Iceberg to see stale metadata when determining which files were committed, resulting in uncommitted manifest files from failed retry attempts being left as orphans instead of being cleaned up automatically. Changes in testInformationSchemaTableAndColumns were required because now INSERTs in the test setup populate the filesystem cache so SELECTs hit the cache.

findepi · 2026-02-18T08:59:51Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java

+
+        public void populateCache(SchemaTableName tableName, Table table)
+        {
+            uncheckedCacheGet(tableCache, tableName, () -> table);


comment why it's safe

raunaqmorarka · 2026-02-18T07:36:41Z

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorSmokeTest.java

+        }
+    }
+
+    private Set<String> listManifestFiles(String tableName)


Can we use computeActual("SELECT path FROM \"table$manifests\"").getOnlyColumnAsSet() ?

findepi · 2026-02-18T09:00:19Z

...c/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergGlueCatalogAccessOperations.java

            assertGlueMetastoreApiInvocations("REFRESH MATERIALIZED VIEW test_refresh_mview_view",
                    ImmutableMultiset.<GlueMetastoreMethod>builder()
-                            .addCopies(GET_TABLE, 6)
+                            .addCopies(GET_TABLE, 7)


findepi · 2026-02-18T09:05:40Z

...c/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergGlueCatalogAccessOperations.java

-                                .add(new FileOperation(METADATA_JSON, "InputFile.length"))
-                                .add(new FileOperation(METADATA_JSON, "InputFile.newInput"))


from commit message

Changes in testInformationSchemaTableAndColumns were required because
now INSERTs in the test setup populate the filesystem cache so SELECTs
hit the cache.

this means the test is no longer testing what it appears it tries to
DESCRIBE still can result in InputFile.newInput, just that it's hidden from the test,
The test should be running with iceberg.metadata-cache.enabled=false, or flush FS cache in assertInvocations

This test class is about testing Glue catalog accesses, and that's what it should focus on.
There are other tests like TestIcebergFileOperations which focus on iceberg metadata accesses on filesystem.

The filesystem access counts depend on catalog in use. In fact, they are here for metadata queries, which have have quite important catalog-specific component. Here we're asserting a metadata query. TestIcebergFileOperations is awesome for regular query access patterns and catalog-agnostic components. It doesn't run with Glue and doesn't, therefore, guard against potential metadata-related regressions in Glue catalog.

I thought any catalog would need to look-up table entry in metastore and then read metadata.json from FS to show output of DESCRIBE. Not sure why the FS access would depend on catalog.

eg

Cache Iceberg column types in Glue for faster access #18315

Accelerate system.metadata.table_comments with Iceberg Glue catalog #18517

github-actions · 2026-03-12T17:18:39Z

This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack.

cla-bot bot added the cla-signed label Feb 12, 2026

github-actions bot added the iceberg Iceberg connector label Feb 12, 2026

piotrrzysko force-pushed the iceberg_glue_cache_fix branch from e796ef4 to 05b11d6 Compare February 12, 2026 09:34

piotrrzysko requested review from findepi and raunaqmorarka February 12, 2026 10:41

piotrrzysko marked this pull request as ready for review February 12, 2026 10:41

raunaqmorarka requested review from ebyhr and findinpath February 12, 2026 11:03

findepi reviewed Feb 12, 2026

View reviewed changes

piotrrzysko force-pushed the iceberg_glue_cache_fix branch 2 times, most recently from d56bb4c to 985fca7 Compare February 16, 2026 08:32

piotrrzysko added 3 commits February 17, 2026 11:40

Add test for uncommitted manifest cleanup

6422ed9

Verify that uncommitted manifest files are properly cleaned up. The test uses concurrent INSERT operations to create conflicts, then checks if any orphaned manifests remain.

Prepare GlueClient for multiple implementations

90567c6

piotrrzysko force-pushed the iceberg_glue_cache_fix branch from 985fca7 to 5cefeca Compare February 17, 2026 10:40

findepi reviewed Feb 18, 2026

View reviewed changes

raunaqmorarka reviewed Feb 18, 2026

View reviewed changes

findepi reviewed Feb 18, 2026

View reviewed changes

github-actions bot added the stale label Mar 12, 2026

		//TODO: Fix https://github.com/trinodb/trino/issues/23941
		abort("Skipped for now due to #23941");

		.add(new FileOperation(METADATA_JSON, "InputFile.length"))
		.add(new FileOperation(METADATA_JSON, "InputFile.newInput"))

Conversation

piotrrzysko commented Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional context and related issues

Release notes

Uh oh!

findepi left a comment

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!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi Feb 18, 2026 • 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!

github-actions bot commented Mar 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

piotrrzysko commented Feb 12, 2026 •

edited

Loading

findepi Feb 18, 2026 •

edited

Loading