ealeonraz commented on issue #12020:
URL: https://github.com/apache/gravitino/issues/12020#issuecomment-5055101999
Thanks @yuqi1129 — this is the right bar, and it clarifies the boundary of
this subtask a lot. Agreed that a single shared Redis copy removes cross-node
divergence but doesn't by itself give read-your-writes against the store;
strong consistency needs the DB↔cache protocol, and several pieces of that live
in the contracts you referenced (#12149 eligibility, #12150 fresh-read, #12151
transactionally-complete change-log) rather than in the cache backend alone. So
I'll scope this implementation as a **shared cache**, not a strongly-consistent
DB/cache pair, and make the design doc say that explicitly.
Here's how I'd address each point:
**Write ordering / completion point.** The cache is only mutated *after* the
store transaction commits, driven by the change-log events from #12151 — never
before or during. A write is "complete" at DB commit; the Redis
fence/invalidation is a post-commit step. This keeps a failed/rolled-back
mutation from ever publishing a cache change (matches #12151's rollback
acceptance criterion).
**DB succeeds but Redis fails/times out.** Fail-safe toward correctness: a
post-commit invalidation that fails must not leave a stale value readable. I'd
invalidate (delete + tombstone) rather than update-in-place, so a Redis failure
degrades to a cache *miss* (fall through to the store), never a stale *hit*.
I'll make the failure policy explicit and configurable — default
fail-open-to-store for reads, but a failed invalidation surfaces as a request
error rather than being silently swallowed. Consistency-sensitive call sites
get their guarantee from #12150's fresh-read path, not from best-effort cache
state.
**Persistent version fence / tombstone.** Already the core of the current
implementation: version-checked writes (`putIfNewer`) and tombstones that
survive value deletion, hierarchical drop, and TTL, so an older in-flight load
can't refill a key that was just invalidated. I'll extend the fence to
explicitly cover **rename** (fence both old and new identifiers, aligned with
#12151's dual-identifier requirement) and make the tombstone lifetime outlive
the value TTL so a slow reader can't win after expiry.
**Redis Cluster hash-slot constraints.** Good catch — this is the real gap.
My hierarchical drop and index operations are multi-key Lua, which under
Cluster requires every key in one slot. I'll bind an entity subtree to a single
slot using a hash tag on the container prefix (e.g.
`{metalake.catalog.schema}...`) so a drop's scan+delete stays atomic and
CROSSSLOT-safe, and document the keyspace layout. Single-key ops are unaffected.
**Tests.** I'll add concurrent reader/writer, delete-then-late-fill,
Redis-failure (invalidation timeout), and retry cases on top of the existing
cross-node-freshness and atomic-drop tests — using Testcontainers, and a
Cluster topology for the hash-slot cases.
One question on sequencing: since this depends on #11961 (interface
simplification) and consumes the #12150/#12151 contracts, do you want the Redis
backend to land after those merge, or land first as an isolated, documented
shared-cache implementation with the fresh-read/change-log integration wired in
as those contracts stabilize? I'm happy either way; I'd lean toward the latter
so it can be reviewed independently, but it's your call on ordering.
--
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]
