ealeonraz commented on PR #13106:
URL: https://github.com/apache/gravitino/pull/13106#issuecomment-6488396084
@yuqi1129 sorry for the slow reply on this one, and thanks for running all
the probes, the eviction repro especially. Yes I'm on it for 2.0.0. Pushed
6770b596a which addresses everything from round 3.
The blocker and two of the important ones had the same root cause (eager
subtree drops plus fences that could expire), so I changed the design instead
of patching each one:
Fence eviction: fences are now one hash per metalake (`<ns>:{m}:F`) with no
TTL, and the generation counter has no TTL either, so `volatile-*` can't evict
them. The fence TTL config and the record age check are gone. At startup the
cache reads `maxmemory-policy` on every primary and refuses `allkeys-*`, and if
`CONFIG` isn't allowed it logs a warning. I added your sequence as a test
against `redis:7.2-alpine --maxmemory 4mb --maxmemory-policy volatile-lru` with
real eviction: the fence and `G` survive and B's fill is rejected.
Unbounded drops: went with the lazy invalidation option. Every value stores
the stamp it was filled under (the fence generations of its identifier and
every ancestor) and a read only serves it if the stamp still matches, otherwise
it deletes it and misses. So `DROP_SCRIPT` is now INCR + HSET + one DEL,
constant time no matter how big the subtree is, and descendants get cleaned up
by their next read or their TTL. Because of that the cache now requires
`expireTimeInMs > 0`. Invalidating the metalake itself also drops that
metalake's fence hash, since the new metalake fence outdates everything under
it anyway. There's a test with 20k cached descendants that checks the drop only
touches the one value.
Registry and read path cost: the IDX index, the registry set and the reaper
are all removed, so a miss is just one EVAL now with no write. `clear()` and
`size()` are documented as maintenance only and scan each primary; `clear()`
fences every metalake it finds but doesn't promise anything for a metalake with
nothing cached. For `CachedEntityIdResolver` I added `EntityCache#peekAll`,
which the Redis cache answers in one script for the whole ancestor chain (one
round trip for a table instead of four) and which doesn't leave a pending
record behind. `contains()` goes through the same stamp check now, otherwise it
could have said true for an outdated entry. I haven't added a benchmark yet, if
you want one in the PR I can add a JMH or simple timing test.
Failed invalidation: `invalidate()` retries once, then logs an error and
counts it and returns false instead of throwing, so the committed write isn't
reported as failed. `RelationalEntityStore` now registers the change log
listener for SHARED caches too, and since every node replays every row this
bounds the failed invalidation, the commit to invalidate crash, and the
failover case by the poll interval. There's a two store test where B's
invalidation fails, B's update still succeeds, and A converges to v2 through
the replay.
Kryo: added a ClassResolver allowlist (`org.apache.gravitino.*` without
anything under `.shaded.`, `java.time`, the boxed types and String,
BigDecimal/BigInteger, the plain JDK collections and the unmodifiable/immutable
wrappers, and Guava immutables). The check runs on the class loaded with
`initialize=false`, before Kryo builds an instance. Also added
`gravitino.cache.redis.ssl`.
Nits: every script declares all its keys in KEYS now (value, fences,
counter, all same slot), and scripts run through EVALSHA with a fallback to
EVAL on NOSCRIPT. The record age nit went away with the fence TTL. Added
`redis-entity-cache` metrics (hits, misses, stale reads, fills written /
rejected / skipped, redis failures, invalidation failures). Added
redis-authx-core to LICENSE.bin and fixed the PR description to say kryo5. The
docs now list the real stale windows, the `maxmemory-policy` requirement and
what an operator sees when an invalidation fails, and the sentence about
skipped fills says what happens with a record left by an earlier miss.
Follow up issue for CACHEABLE_TYPES: #14345
Tested locally: core unit tests, the script tests (7), standalone IT (30),
cluster IT (29 against a 3 primary 3 replica cluster), the two store suite (5)
and the serializer tests (15), all green.
--
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]
