yuqi1129 opened a new issue, #12376: URL: https://github.com/apache/gravitino/issues/12376
### What would you like to be improved? The entity store cache (`gravitino.cache.enabled`, default true) is a per-node cache kept coherent by the `entity_change_log` poller. Two problems make cached reads stale in ways that affect behavior, not just freshness. **1. `batchGet` can write a stale entity back after it was invalidated.** `RelationalEntityStore.get()` holds the cache's segment lock across the backend read and the write-back, so it cannot race with the poller's `invalidate`. `RelationalEntityStore.batchGet()` does not: it reads from the backend and then calls `cache.put()` unlocked. If the poller invalidates a key between the two, the stale entity is written back and survives until the cache TTL (`gravitino.cache.expireTimeInMs`, default 1 hour) instead of one poll interval. This path is on the request hot path: `MetadataAuthzHelper` uses it to preload entities for authorization. **2. Four reads use cached values to decide whether to perform an action.** A stale value on these paths does not just return old data, it changes what the server does: | Call site | Effect of a stale read | | --- | --- | | `MetalakeManager.metalakeInUse()` | the guard behind 23 `checkMetalake(...)` call sites admits writes to a metalake already disabled on another node | | `PolicyManager.changePolicyEnabledState()`, via `policyEnabled()` | the short-circuit returns without doing anything, so enable/disable is silently dropped while the API reports success | | `JobManager.cancelJob()` | a stale terminal status skips `jobExecutor.cancelJob()`, leaving the external job running | | `FilesetCatalogOperations.getFileLocation()` | returns a usable storage path for a fileset already dropped on another node | ### How should we improve? - Make `batchGet` unable to write back an entity that was invalidated after its backend read. - Add an explicit fresh-read channel on `EntityStore` that reads from the backend under the cache's segment lock and refreshes the entry, and use it at the call sites above. Bypassing the cache ad hoc at each call site is deliberately avoided so that the strongly consistent reads stay greppable and reviewable. - Keep every other read cached: those are bounded-staleness reads whose result is returned to the caller rather than used to branch. Tracked separately, not in scope here: - The `entity_change_log` poller advances its cursor with `WHERE id > lastConsumedId` over an auto-increment id, so a row committed out of id order can be skipped permanently, which turns a one-poll-interval staleness window into a one-TTL window. - `PolicyManager` and `JobManager` perform check-then-act across nodes, which is racy even with the cache disabled because `TreeLockUtils` locks are in-process only. A conditional update is the real fix there; a fresher read only narrows the window. -- 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]
