feat(rest): support freshness-aware table loading by YuangGao · Pull Request #3377 · apache/iceberg-rust

YuangGao · 2026-10-09T04:28:13Z

Which issue does this PR close?

Closes RestCatalog: support freshness-aware table loading (If-None-Match / 304 Not Modified), as Iceberg Java does #3340.

What changes are included in this PR?

load_table never sent If-None-Match and parsed a body-less 304 as a 200. Following Java (apache/iceberg#14398):

Cache tables loaded with an ETag, keyed by session and identifier; a later load sends If-None-Match and returns the cached table on 304.
Add rest-table-cache.max-entries (default 100, 0 disables) and rest-table-cache.expire-after-write-ms (default 5 minutes).

Only load responses fill the cache, as in Java: the reference server's ETag also covers query parameters, so a create response's never matches a later load.

Are these changes tested?

Unit tests with mockito.
Verified against apache/iceberg-rest-fixture:latest: loads return 200 then 304; after a commit, drop or re-create, the stale ETag yields 200 or 404 instead of the cached table.

AI Disclosure

AI-assisted implementation.

JeonDaehong · 2026-10-10T04:34:10Z

+        };
+        Ok(TableCache::builder()
+            .max_capacity(parse(REST_CATALOG_PROP_TABLE_CACHE_MAX_ENTRIES, 100)?)
+            .time_to_live(Duration::from_millis(parse(


Compared with Java's RESTTableCache: the property names, defaults, the (session id, identifier) key and the 304 handling all match. One difference: Java rejects rest-table-cache.expire-after-write-ms of 0 (Preconditions.checkArgument(expireAfterWriteMS > 0, "Invalid expire after write: zero or negative")), while here 0 is accepted. Should this reject 0 too, or document what 0 does?

good catch, thanks. 0 is now rejected like in Java, since it's ambiguous (disable vs never expire) and max-entries=0 already turns the cache off. while there, also capped it at 1000 years, which moka would otherwise panic on.

Thanks, the upper bound is a nice catch too.

laskoviymishka

Nice to see the 304 handling fixed properly — the old path deserializing a body-less 304 as a LoadTableResult was a latent bug, and the If-None-Match flow reads cleanly end to end. The TTL-range validation JeonDaehong flagged looks settled (rejecting 0 and capping at 1000 years → DataInvalid, with a test), so I won't relitigate that.

I don't think this should merge yet, though, because a 304 returns a Table whose FileIO was frozen at the original load. For any catalog that vends short-lived storage credentials in the loadTable config, a cache hit hands back credentials captured up to one TTL ago — fine under the 5-minute default, but the TTL is configurable up to 1000 years, and anything vending tokens shorter than the TTL gets 403s from object storage on a "successful" load. Those surface from the FileIO rather than the catalog, so they're nasty to diagnose. Java's RESTSessionCatalog doesn't serve a stale FileIO on a hit (apache/iceberg#14398) — I'd cache the LoadTableResult config and rebuild per hit, or skip caching when the response carried credential-bearing config.

Second, we invalidate the cache nowhere. Correctness mostly survives on the server's ETag check, but a dropped-and-recreated table at the same ident is only caught if the server never reuses an ETag for recreated content, and the spec treats the ETag as opaque. Java invalidates by SessionIdTableId on drop/rename/commit/register and on 404 — I'd match that.

Before merge, I'd want:

304 not handing back stale vended credentials (rebuild FileIO per hit, or don't cache credential-bearing loads), plus a doc note on the TTL / credential-lifetime interaction.
Invalidation on drop/rename/register/commit and the 404 branch, and the "stale entries are harmless" doc line softened.
test_table_cache_is_per_session made discriminating (right now it passes even with caching broken), and the revalidation test asserting the actual cached table rather than a shared fixture's metadata_location.

The rest — moka under [workspace.dependencies], no-ETag-200 eviction, hot-path work when the cache is disabled, named consts for the magic numbers — is non-blocking and noted inline. Once the credential and invalidation pieces land I'll take another pass.

laskoviymishka · 2026-10-11T18:42:27Z

 iceberg = { workspace = true }
 itertools = { workspace = true }
+moka = { version = "0.12.10", features = ["sync"] }
 reqwest = { workspace = true }


The iceberg crate already pins moka (with the future feature), so I'd move the version into [workspace.dependencies] and use moka = { workspace = true } here rather than carry two independent pins. Not a blocker.

moka = { workspace = true, features = ["sync"] }

updated, thanks.

laskoviymishka · 2026-10-11T18:42:27Z

+
+/// Tables loaded with an `ETag`, keyed by session id and identifier. Stale
+/// entries are harmless: the server answers `304` only while the `ETag` matches.
+type TableCache = moka::sync::Cache<(String, TableIdent), (String, Table)>;


This "stale entries are harmless" claim leans harder than it looks. We invalidate nowhere — not on drop, rename, register, commit, or a 404 load — so a dropped-and-recreated table at the same ident is only caught if the server never reuses an ETag for recreated content. The spec treats the ETag as opaque, so a server deriving it from the metadata hash alone would 304 against the pre-drop entry and hand back a Table whose FileIO/credentials predate the recreate. Correctness otherwise holds (the server still 404s a dropped table), but the dropped table's Table stays pinned in memory until the TTL, and a 404 load leaves the stale If-None-Match in place instead of evicting.

Java invalidates by SessionIdTableId on drop/purge/rename/commit/register and on NoSuchTable. I'd add invalidate(&key) to delete_table, rename_table (src), register_table, update_table, and the 404 branch, and soften this doc line. One fork worth deciding: invalidate per-session like Java, or across sessions via invalidate_entries_if on the ident? Thoughts?

fair point, the ETag is opaque. added invalidation on drop/purge, rename (source), register, commit and the 404 branch, and softened the doc. went per-session like Java: cross-session would need invalidate_entries_if, which has to be enabled up front and scans entries on each call.

laskoviymishka · 2026-10-11T18:42:27Z

+        )?);
+        // Zero is ambiguous (disable or never expire), and moka panics beyond 1000 years.
+        if expire_after_write.is_zero()
+            || expire_after_write > Duration::from_secs(1000 * 365 * 24 * 3600)


1000 * 365 * 24 * 3600 reads as a magic number and couples us to moka's internal 1000-year panic bound — I'd lift it to a named const MAX_TABLE_CACHE_TTL: Duration, alongside named consts for the 300_000 and 100 defaults below. While here, the error drops the offending value (unlike the parse error); including {expire_after_write:?} would make a bad config easier to spot.

updated, thanks.

laskoviymishka · 2026-10-11T18:42:27Z

        table_ident: &TableIdent,
    ) -> Result<Table> {
        let client = self.client().await?;
+        let cache_key = (context.session_id().to_string(), table_ident.clone());


Worth documenting that the cache is keyed on session_id, so it only helps when callers reuse a SessionContext — SessionContext::empty() mints a fresh id each call, so those callers get a 0% hit rate while still paying the key allocation and the table clone. Java keys the same way, so this is a doc note rather than a bug — I'd just call it out on the property constants so a multi-tenant proxy spinning a context per request knows what to expect.

updated, thanks.

laskoviymishka · 2026-10-11T18:42:27Z

-                deserialize_catalog_response::<LoadTableResult>(http_response)?
+            StatusCode::OK => deserialize_catalog_response::<LoadTableResult>(http_response)?,
+            StatusCode::NOT_MODIFIED => {
+                return cached.map(|(_, table)| table).ok_or_else(|| {


A 304 hands back the cached Table, and that Table carries a FileIO built from the earlier 200's config — including any vended s3.session-token / gcs.oauth2.token. The ETag only validates the metadata, and a 304 has no body to refresh credentials from, so on a hit we return storage credentials captured up to one TTL ago. With the 5-minute default that's usually under the STS lifetime, but the TTL is configurable up to 1000 years — any catalog vending tokens shorter than the TTL gets 403s from object storage on a "successful" load, and those surface from the FileIO, not the catalog, so they're painful to trace.

Java's RESTSessionCatalog doesn't serve a stale FileIO on a hit (apache/iceberg#14398) — please confirm the exact mechanism. The cleanest fix here is to cache the LoadTableResult config and rebuild the FileIO per hit; failing that, skip caching when the 200 carried credential-bearing config, and document that expire-after-write-ms must stay under the credential lifetime.

good catch. checked Java: on a hit it rebuilds the FileIO, but from the credentials captured at the original load (a 304 has no body). it stays fresh because VendedCredentialsProvider refreshes them via client.refresh-credentials-endpoint, and iceberg-rust has no refresh yet (#2932), so rebuilding per hit wouldn't help here. went with not caching loads that vend credentials (storage-credentials, or a secret-looking config key via is_sensitive_prop). we can revisit once refresh lands.

laskoviymishka · 2026-10-11T18:42:27Z

            table_builder.build()
+        }?;
+
+        if let Some(etag) = etag {


On a 200 that comes back without an ETag (a server that stopped sending it, or a proxy that strips it) we neither replace nor drop the existing entry, so every subsequent load keeps sending the stale If-None-Match. I'd add an else arm that invalidates:

if let Some(etag) = etag { client.table_cache.insert(cache_key, (etag, table.clone())); } else { client.table_cache.invalidate(&cache_key); }

updated, thanks.

laskoviymishka · 2026-10-11T18:42:27Z

+        }?;
+
+        if let Some(etag) = etag {
+            client.table_cache.insert(cache_key, (etag, table.clone()));


When the cache is disabled (max-entries=0) we still allocate the key, clone the Table, and call insert here only for moka to drop it immediately — and the ETag string is built on every response, including 304s and errors. I'd store Option<TableCache> (None when max is 0) and skip the whole path, and move the ETag extraction into the OK arm so it's only built when we might actually cache. Non-blocking perf tidy.

moved the ETag extraction into the 200 arm. kept the cache non-optional though: moka already short-circuits get/insert at max_capacity 0, and with invalidation on several paths an Option would add a branch to each.

laskoviymishka · 2026-10-11T18:42:27Z

+        mock.create_async().await
+    }
+
+    fn test1() -> TableIdent {


test1() reads like a test rather than a fixture — test_table_ident() (or inlining it) would be clearer. Minor, and while in these tests mockito::Matcher gets spelled out in full repeatedly; a use would tidy them up.

updated, thanks.

laskoviymishka · 2026-10-11T18:42:27Z

+
+        let loaded = catalog.load_table(&context, &test1()).await.unwrap();
+        let reused = catalog.load_table(&context, &test1()).await.unwrap();
+        assert_eq!(loaded.metadata_location(), reused.metadata_location());


Both responses use the same load_table_response.json, so comparing metadata_location only proves a table came back — not that the cached table was the one returned, nor that the v2 200 actually replaced v1. I'd give v1 and v2 distinct metadata_locations and assert the reused table equals v1 after the 304 and v2 after the second 200.

updated, thanks.

laskoviymishka · 2026-10-11T18:42:27Z

+    }
+
+    #[tokio::test]
+    async fn test_table_cache_is_per_session() {


This passes even if caching were completely broken — two loads with no If-None-Match is exactly what test_table_cache_disabled_with_zero_max_entries asserts, so the two are indistinguishable. It also hinges on SessionContext::empty() handing out distinct ids; if that ever became deterministic the second load would send If-None-Match, the Missing matcher would miss, and mockito would 501. I'd make it discriminating: load in session A, load in session B and assert B sent no header, then load A again and assert it sent If-None-Match and got a 304 — with explicit session ids rather than relying on empty().

updated, thanks.

laskoviymishka

Thanks for turning this around — a lot landed. Invalidation now fires on drop/purge, rename, register and commit plus the 404 branch, the no-ETag 200 evicts, and credential-bearing loads are skipped so a 304 can't hand back a frozen FileIO. The 304-with-empty-body path is gone too, with the Unexpected case now tested. And both tests I pushed on last round are discriminating now — test_table_cache_is_per_session fails if the key drops the session, and the revalidation test actually asserts v1-then-v2 off the cache. That's the shape I was after.

The TTL range check is settled — rejecting 0 and capping at 1000 years is exactly right, not reopening that.

Two things are still open from last round. First, the invalidation calls all sit right after query_catalog(...)?, so a transport error — the server applied the drop or commit but we saw a timeout or reset — takes the ? and skips the invalidation, leaving the stale entry. That's the ambiguous-outcome case, and it's the drop-and-recreate hazard I flagged in round 1; the ETag check only saves us as long as the server never reuses an ETag. I'd move the invalidate before we send the mutating request, or onto a path that runs on both success and error — it's idempotent, so both is fine.

Second, vends_credentials leans on is_sensitive_prop, which is a log-redaction denylist rather than a credential classifier. Being substring- and case-based, it over-skips static keys like s3.sse.key and encryption.key-id (those tables silently lose caching) and can under-skip an oddly-named short-lived cred. The type-alias doc also says we skip when a load "vends storage credentials", but the code skips on any sensitive-named config key — I'd give this its own named predicate and match the doc to it.

Before merge, I'd want:

the invalidation moved so it runs on the transport-error path too;
a dedicated credential predicate, with the doc lined up to it;
tests for the new invalidation one-liners — rename/register/commit and the 404 branch are all untested, and the _ => invalidate arm (a cached entry that later starts vending creds) is too, so a regression there passes silently.

Smaller stuff, none of it blocking: the self-healing insert-after-invalidate race is worth a one-line comment rather than a fix (Java's cache has the same shape); a note on the props that invalidation is per-session; and with max-entries=0 we still build the key and clone/insert on every load, which an Option<TableCache> would skip. The validation error also prints a Duration Debug (0ns) instead of the ms the user actually passed — easy to fix while you're in there.

This is close now — get the ordering and the credential predicate sorted and I'm happy to pull it through.

laskoviymishka · 2026-10-12T21:06:24Z


        let request = HttpRequest::build(request_builder)?;
        let http_response = client.query_catalog(context, request).await?;
+        client.invalidate_table(context, table);


I'd move this to before we send the mutating request (or onto a path that runs on error too): as written it sits after query_catalog(...)?, so a transport error — the server applied the drop but we saw a timeout or reset — takes the ? and skips the invalidation, leaving the stale entry. That's the ambiguous-outcome case, and the exact drop-and-recreate hazard from last round; the If-None-Match check only saves us while the server never reuses an ETag. Same ordering applies to rename, register and commit — invalidate is idempotent, so running it on both paths is fine.

moved the invalidation before the request on drop/purge, rename, register and commit, so it also runs when the request fails in transit.

laskoviymishka · 2026-10-12T21:06:24Z

+            .storage_credentials
+            .as_ref()
+            .is_some_and(|credentials| !credentials.is_empty())
+            || response.config.keys().any(|key| is_sensitive_prop(key));


is_sensitive_prop is a log-redaction denylist, not a credential classifier, and reusing it here cuts both ways: it over-skips static keys like s3.sse.key and encryption.key-id (those tables silently lose caching) and can under-skip an oddly-named short-lived cred. I'd give the cache its own predicate, ASCII-lowercased and scoped to the keys that actually carry short-lived creds. The type-alias doc also says we skip when a load "vends storage credentials", but the code skips on any sensitive-named config key — worth lining the doc up with what it actually does.

added a dedicated is_short_lived_credential: s3.session-token, gcs.oauth2.token and adls.sas-token.*, ASCII-lowercased, the same keys Java treats as short-lived. static keys like s3.sse.key are cached again, and the doc now says short-lived credentials.

laskoviymishka · 2026-10-12T21:06:24Z

+    }
+
+    #[tokio::test]
+    async fn test_drop_table_invalidates_cached_table() {


Good to have drop covered — I'd extend the same load/mutate/load shape to the other invalidation calls this round added: rename, register, commit and the 404 branch are all untested one-liners that regress silently. The _ => invalidate arm is the other gap: a cached entry whose next 200 starts vending creds should evict, and nothing exercises that today — two credential-vending 200s both pass even if the arm were deleted, since nothing was ever cached. A storage_credentials case alongside the config-key one would close it.

replaced the drop test with a load/change/load test across drop, rename, register and commit, and added one for the 404 branch and one where a cached entry's next 200 vends storage-credentials and gets evicted.

laskoviymishka

This is good to merge as is.

The three inline ones are optional follow-ups, none of them need to hold the PR. If you pick one up, I'd start with the test that pins the before-send ordering, since right now nothing would catch the invalidate moving back below the ?. The public prop doc line and the predicate test are nice-to-haves.

laskoviymishka · 2026-10-15T18:23:29Z

+/// Maximum number of loaded tables kept for freshness-aware loading (defaults
+/// to 100; 0 disables the cache). The cache is keyed by session id, so it only
+/// helps callers that reuse a [`SessionContext`], and a table changed through
+/// this catalog is invalidated only for the session that changed it.


The credential skip is only explained on the private TableCache alias. For a catalog that vends credentials on every load, this property does nothing, so I'd say that here where users will see it.

Suggested change

/// this catalog is invalidated only for the session that changed it.

/// this catalog is invalidated only for the session that changed it. Loads that

/// vend short-lived storage credentials are never cached.

done, thanks.

laskoviymishka · 2026-10-15T18:23:29Z

+/// Whether a `loadTable` config key carries a short-lived storage credential.
+fn is_short_lived_credential(key: &str) -> bool {
+    let key = key.to_ascii_lowercase();
+    key == S3_SESSION_TOKEN || key == GCS_TOKEN || key.starts_with(ADLS_SAS_TOKEN)


I'd add a small table-driven test for this. Only the s3.session-token arm runs today, through the vended-credentials tests. Use gcs.oauth2.token, an adls.sas-token.<account> key and a mixed-case S3.Session-Token as positives, and s3.sse.key, s3.access-key-id and encryption.key-id as negatives, so last round's over-skip can't quietly come back.

added a table-driven test with those positives and negatives.

laskoviymishka · 2026-10-15T18:23:29Z

        }

        let request = HttpRequest::build(request_builder)?;
+        client.invalidate_table(context, table);


Nothing pins this ordering yet. Every mutation mock returns a clean status, so moving invalidate_table back below query_catalog(...)? keeps the suite green. I'd add one case where the drop errors out of query_catalog itself, then assert that the next load sends no If-None-Match. A 5xx won't catch it, because that's still Ok(response).

added one: the drop's response is cut off mid-body, so it fails inside query_catalog, and the next load sends no If-None-Match. it fails if the invalidation moves back below the ?.

feat(rest): support freshness-aware table loading

69d9a9a

YuangGao force-pushed the feat/rest-freshness-aware-load branch from bbfe47e to 69d9a9a Compare October 9, 2026 04:34

YuangGao mentioned this pull request Oct 9, 2026

feat: support freshness-aware table loading in RestCatalog apache/iceberg-python#4094

Open

JeonDaehong reviewed Oct 10, 2026

View reviewed changes

fix(rest): reject out-of-range table cache expiry

ea340dd

laskoviymishka requested changes Oct 11, 2026

View reviewed changes

fix(rest): address review feedback on table cache

5e82c75

laskoviymishka requested changes Oct 12, 2026

View reviewed changes

fix(rest): invalidate before sending changes and narrow credential check

5763296

laskoviymishka approved these changes Oct 15, 2026

View reviewed changes

test(rest): pin table cache invalidation order and credential keys

54c79ba

	/// this catalog is invalidated only for the session that changed it.
	/// this catalog is invalidated only for the session that changed it. Loads that
	/// vend short-lived storage credentials are never cached.

Conversation

YuangGao commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

AI Disclosure

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!

laskoviymishka 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!

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!

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!

laskoviymishka 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!

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!

laskoviymishka 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

YuangGao commented Oct 9, 2026 •

edited

Loading