uros-b commented on code in PR #17375:
URL: https://github.com/apache/iceberg/pull/17375#discussion_r3652149746
##########
core/src/main/java/org/apache/iceberg/CachingCatalog.java:
##########
@@ -144,29 +144,41 @@ public Table loadTable(TableIdentifier ident) {
return cached;
}
- Table table = tableCache.get(canonicalized, catalog::loadTable);
-
- if (table instanceof BaseMetadataTable) {
- // Cache underlying table
- TableIdentifier originTableIdentifier =
- TableIdentifier.of(canonicalized.namespace().levels());
- Table originTable = tableCache.get(originTableIdentifier,
catalog::loadTable);
-
- // Share TableOperations instance of origin table for all metadata
tables, so that metadata
- // table instances are refreshed as well when origin table instance is
refreshed.
- if (originTable instanceof HasTableOperations) {
- TableOperations ops = ((HasTableOperations) originTable).operations();
- MetadataTableType type = MetadataTableType.from(canonicalized.name());
-
- Table metadataTable =
- MetadataTableUtils.createMetadataTableInstance(
- ops, catalog.name(), originTableIdentifier, canonicalized,
type);
- tableCache.put(canonicalized, metadataTable);
- return metadataTable;
- }
- }
-
- return table;
+ // Use asMap().compute() atomically so that any concurrent invalidate()
will not race with a
+ // put that would otherwise resurrect a stale value in the cache. See
issue #17338.
+ return tableCache
+ .asMap()
+ .compute(
+ canonicalized,
+ (key, existing) -> {
+ if (existing != null) {
+ return existing;
+ }
+
+ Table table = catalog.loadTable(key);
+
+ if (table instanceof BaseMetadataTable) {
+ // For metadata tables, share the TableOperations of the
origin table so that
+ // metadata table instances are refreshed as well when the
origin table instance
+ // is refreshed. The origin table is loaded (and cached) via a
separate atomic
+ // compute below.
+ TableIdentifier originTableIdentifier =
+ TableIdentifier.of(key.namespace().levels());
+ Table originTable =
+ tableCache
+ .asMap()
+ .computeIfAbsent(originTableIdentifier,
catalog::loadTable);
Review Comment:
The metadata branch performs this nested call from inside the outer
compute(canonicalized, …) remapping function, mutating the same cache during
its own computation. This is the anti-pattern PR #3801 deliberately removed
after production deadlock #3791; both Caffeine's asMap() view and the CHM it
wraps document that the mapping function must not update the map during
computation. Depending on interleaving and key hashing, it can deadlock or
throw a "Recursive update" IllegalStateException, and the still-synchronous
RemovalListener firing invalidateAll(...) during eviction compounds the
liveness surface. The branch is reachable by any ordinary metadata-table load
(e.g. table.snapshots, table.files), so this is a common path, not an edge
case. Merging as-is risks regressing #3791.
--
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]
