apache / apache/gravitino

[Improvement] Fix stale entity cache reads: batchGet write-back race and action-sensitive reads

Open
#12,376 0 comments 0 reactions 0 assignees View on GitHub
improvement
Dominant language
Java
Stars
3.2k
Forks
935
Avg merge
1d 16h
Merged PRs (30d)
298

Description

### 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.

Contributor guide

Open the contributing guide

Research direction

Start with RelationalEntityStore.get() and batchGet(), then inspect the EntityStore interface and the entity_change_log poller to understand cache invalidation. Trace the four named call sites: MetalakeManager.metalakeInUse(), PolicyManager.changePolicyEnabledState(), JobManager.cancelJob(), and FilesetCatalogOperations.getFileLocation(). Done means batchGet cannot restore an invalidated value and those action-sensitive reads use the explicit fresh-read path while other reads remain cached.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
authorization, backend, distributed-systems
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.