morningman commented on code in PR #68860:
URL: https://github.com/apache/doris/pull/68860#discussion_r4236519503
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonCatalogOps.java:
##########
@@ -308,11 +308,9 @@ public List<String> listTables(String databaseName) throws
Catalog.DatabaseNotEx
*/
@Override
public Table getTable(Identifier identifier) throws
Catalog.TableNotExistException {
- // Doris owns the outer table-metadata cache and only reaches this
method on an outer-cache
- // load. Paimon's CachingCatalog owns a second cache for the Table
and its lower-level readers.
- // Invalidate that inner entry before reloading, otherwise REFRESH
or outer-cache expiry can
- // immediately repopulate Doris with the same stale bucket, path,
or table options.
- catalog.invalidateTable(identifier);
+ // Every metadata, scan and write lookup lands here, several times
per statement. The wrapped
Review Comment:
The two lifetimes were already separate before this PR, and branch-4.x has
the same staleness. Since the connector cutover (#64304) the latest-snapshot
pin has followed `meta.cache.paimon.table.ttl-second` (an access TTL, 24 h by
default). Setting it to 0 turns off both the table cache and the pin, which is
what the no-cache catalog in `test_paimon_table_meta_cache` does.
`paimon.cache-enabled` and `paimon.cache.expire-after-*` are Paimon's own
catalog-cache options. #67996 applied them to the Doris table cache when it
replaced the SDK `CachingCatalog`, but they never governed the pin. On
branch-4.x they configure only the SDK `CachingCatalog` (4.x even turns it off
under a weight limit). Doris keeps serving its cached snapshot there until the
`meta.cache.paimon.table` TTL or REFRESH, and that TTL is access-based too, so
frequent reads keep it alive there as well.
The per-lookup invalidation added by #67395 hid this, but it also kept the
cache from ever hitting (0 hits in 86 lookups for 7 SELECTs), which is the bug
this PR fixes. Making `paimon.cache-enabled` also turn off the snapshot pin
would give the option a meaning it does not have on branch-4.x, so I'd keep
that out of this PR.
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonCatalogOps.java:
##########
@@ -308,11 +308,9 @@ public List<String> listTables(String databaseName) throws
Catalog.DatabaseNotEx
*/
@Override
public Table getTable(Identifier identifier) throws
Catalog.TableNotExistException {
- // Doris owns the outer table-metadata cache and only reaches this
method on an outer-cache
- // load. Paimon's CachingCatalog owns a second cache for the Table
and its lower-level readers.
- // Invalidate that inner entry before reloading, otherwise REFRESH
or outer-cache expiry can
- // immediately repopulate Doris with the same stale bucket, path,
or table options.
- catalog.invalidateTable(identifier);
+ // Every metadata, scan and write lookup lands here, several times
per statement. The wrapped
+ // catalog is the Doris table cache (PaimonMetaCacheCatalog; the
SDK CachingCatalog is disabled),
+ // so this must not invalidate: REFRESH, Doris writes and DDL
invalidate through CatalogMetaCache.
Review Comment:
This matches branch-4.x, where #67023 made the Doris metadata cache the
owner of the table instances that writes use, so a column another engine adds
becomes writable only after REFRESH TABLE or cache expiry there too. What
differs on master is that `getTableSchema` reads the latest schema file rather
than the cached table's `rowType()` (a stale `rowType()` kept old names after a
RENAME), so with `schema.cache.ttl-second=0` DESC can show the column before
INSERT accepts it. That window ends at REFRESH TABLE or when the cached table
expires, at most Paimon's `cache.expire-after-write` (30 minutes by default).
Reloading the table for each write would have to give every handle in the
statement the same table version, since the write plan and the bound write
columns both read the table carried by their handle. That is a change to the
write path's metadata contract rather than to this PR's cache hits, so I'd
rather not fold it in here.
--
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]
