humzahkiani opened a new pull request, #17341:
URL: https://github.com/apache/iceberg/pull/17341
## Problem
`CachingCatalog` hands out `Table` instances pinned to the snapshot that was
current when the table was loaded, and a commit performed through such a
table
is invisible to the cache. The cached entry is therefore never updated on
commit, which allows a stale snapshot to be served after a commit:
1. A writer loads a table (snapshot N) and begins a long-running write.
2. The cache entry expires mid-write (the default TTL is 30s).
3. Another thread (for example an OpenLineage Spark listener) calls
`loadTable`,
reloads snapshot N (pre-commit), and re-caches it with a fresh TTL.
4. The writer commits snapshot N+1 through its own reference. Nothing
invalidates the cache.
5. A subsequent `loadTable` returns the re-cached snapshot N, a stale read.
This is the race reported in #10493. It caused a production stale-read
incident
for us after an OpenLineage listener began issuing background `loadTable`
calls
during writes.
## Fix
Extend the invalidate-on-commit behavior that already exists for
`buildTable(...).replaceTransaction()` and `createOrReplaceTransaction()` to
tables returned by `loadTable`. On a cache miss, the loaded `BaseTable` is
rebuilt with a `TableOperations` wrapper that invalidates the table's cache
entry (and its metadata tables) after a successful `commit`. All write paths,
including direct operations (`newAppend()`, `newOverwrite()`, and so on),
`newTransaction()`, and metadata-table operations that share the origin
table's
ops, funnel through `TableOperations.commit`, so a single interception point
covers them.
The wrapper keeps the returned object a `BaseTable`, so `instanceof
BaseTable`
and `(BaseTable)` consumers across core, REST, and the engines are
unaffected.
Only tables loaded on a cache miss are wrapped; tables served directly from
the
cache after `createTable` keep their current behavior, so no existing
behavior
or tests change. The same wrapper could be applied to the create path in a
follow-up if desired.
### Race safety
The load path uses Caffeine's atomic `Cache.get(key, loader)`, where the
value
is loaded and stored under the per-key compute lock, and the wrapper
invalidates
only after the commit is durable. Across all interleavings, a concurrent
reload
either has its stored entry invalidated by the commit, or (if it runs after
the
invalidation) reads the post-commit snapshot. A `CommitStateUnknownException`
also triggers invalidation, since the commit may have succeeded.
## Tests
Adds `testLoadTableObservesCommitAfterConcurrentReloadDuringWrite`, a
deterministic regression test that reproduces the interleaving above (expired
entry, concurrent reload, then commit) and asserts the post-commit
`loadTable`
observes the committed snapshot. It fails without the fix and passes with it.
All existing `TestCachingCatalog` tests pass unchanged.
Closes #10493
--
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]
