ealeonraz commented on PR #13106:
URL: https://github.com/apache/gravitino/pull/13106#issuecomment-5747818130
Thanks for the detailed review and for actually reproducing these, that made
them a lot easier to chase down. Pushed a commit that addresses each point.
Summary by finding:
**[P1] Missing fence snapshots must not authorize unconditional writes.**
Fills now fail closed. `doPut` only writes when a miss on the same thread
recorded the fences that bound the load; with no record (insert-time warming, a
record evicted from the per-thread bound, a read that failed or hit an
undecodable entry) nothing is written and the next read loads under a fresh
record. A failed read also drops any earlier record for that key so it cannot
vouch for a load it did not bound. Insert no longer warms the cache; the first
read does. Regression coverage: the insert/delete race, 1,025 misses before
fill, read failure followed by recovery (pausing the container), and decode
failure followed by a concurrent invalidation, plus unit tests on the
bookkeeping.
**[P1] Expiring counters can reuse a fence version.** Fence values are now
generations from a per-metalake counter (`<ns>:{metalake}:G`) that never
expires, so a recreated fence can never carry a value an older read observed.
Fence keys can still expire, and to make that safe a fill whose record is older
than `fenceTtlMs` is discarded on the client, which closes the absent, set,
expired, absent sequence too: the only way to see "absent" twice across an
expiry is to be older than the fence lifetime. So the fence TTL is now the
explicit safe lifetime of a fill record rather than a hope that fences outlive
readers. Both ABA sequences are tests now, at value TTL 100 ms and fence TTL
200 ms.
**[P1] Clearing values and the index separately.** `clear()` is one script
per metalake: it moves the metalake's own fence to a fresh generation (every
fill in flight for that metalake is rejected, since every fill checks the
metalake fence) and deletes the values and the index in the same script.
`discardQuietly` is also a script now and only removes the entry if it still
holds the bytes that were read, so a concurrent fill is never unindexed. Test:
clear between a miss and its fill, then a metalake invalidation, then a scan
for orphaned value keys.
**[P2] Expired values leave index members indefinitely.** Added a bounded
reaper script: `ZRANGEBYLEX` a batch from a cursor, `EXISTS` each value, `ZREM`
the missing ones, all inside one script so a refill that landed first is never
removed. It runs one batch per 64 writes into a metalake and one batch per
index visited by `size()`, resuming where it left off. Test: expired members
reclaimed, and a refill survives the reaper.
**[P2] Wire Redis client closure into the entity-store lifecycle.** Added
`EntityCache.close()` with a default of `clear()`, so Caffeine keeps
clear-on-close, and `RelationalEntityStore.close()` now calls it. The Redis
implementation closes only its own client, and every operation after `close()`
fails with an `IllegalStateException`: the cluster client otherwise reconnects
on demand after being closed, which is the "still usable" behavior you saw.
Store-level test: closing one store leaves the other store's entries, plus unit
and IT coverage that close never touches data and that a closed cache rejects
further use.
**[P2] Iterate cluster primaries rather than every discovered node.**
Node-wide scans now check `INFO replication` and skip replicas; every per-key
command that follows a scan (`ZCARD`, the clear script) goes through the
cluster client so a redirection is handled rather than thrown. The cluster IT
now starts three primaries with one replica each.
**[P2] Namespace scanning can delete another deployment's cache.** The scan
pattern is anchored on the `:{` that always follows the namespace, ownership of
every scanned key is checked exactly before it is touched, glob characters are
escaped, and the namespace config is restricted to letters, digits and `._-:`
so a metacharacter cannot get in. Tests for `ns` vs `ns:other` on clear and for
rejected namespaces.
**Tests.** The concurrency test now commits distinct versions and asserts no
reader ever observes a version older than an invalidation that completed before
its read began. Added `TestRelationalEntityStoreRedisCache`, which runs two
real `RelationalEntityStore`s over one H2 and one Redis through insert, get,
batchGet, update and close, with a latch pausing one store between its backend
load and its cache fill while the other commits and invalidates. Also
`TestRedisEntityCacheScripts` for the Lua scripts at interleavings the API
cannot produce on demand.
**CI.** The cluster IT failed to initialize because it ran the cluster
container on the host network, which is not reachable the same way everywhere.
It now runs on the default bridge network and connects to the container's own
address, which a Linux Docker host (the CI runners) routes to; on a host that
cannot (Docker Desktop) the suite skips with the reason instead of failing, and
`GRAVITINO_REDIS_CLUSTER_ADDRESS` still points it at an external cluster. If
the cluster never becomes ready the failure now carries the container logs.
On `CACHEABLE_TYPES`: I would keep that out of this PR. The reason USER,
GROUP and ROLE are excluded is not the per-node copies, it is that their
materialized form embeds relation data that no write path invalidates (renaming
a securable object never calls `invalidate` on the roles that reference it). A
shared copy does not change that, so making them cacheable needs those write
paths to invalidate the principals they affect, which is its own change. Happy
to open a follow-up issue for it.
--
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]
