laserninja commented on PR #12470:
URL: https://github.com/apache/gravitino/pull/12470#issuecomment-5472661180
Thanks @nevzheng, and no worries about the delay. All four items are
addressed in `ee89c9f`. Each of the two blocking ones turned out to be a live
defect rather than only a gap in coverage, and the fourth surfaced a third.
**Case variants (blocking).** Confirmed: `reserve("017F22E2-…")` and
`reserve("017f22e2-…")` hashed to different entries, so both won `RESERVED` and
the mutation ran twice — and the spec's own example is upper case, so this was
reachable by a spec-conformant client. Added `IdempotencyKeys.canonicalize`,
folding with `Locale.ROOT`, applied at the store boundary rather than trusting
callers. This also matters for the JDBC store, where `utf8mb4_bin` is
case-sensitive; only the folded form is ever written.
**Fencing (blocking).** Confirmed, and worse on the release path than the
finalize path. `release()` deleted whatever record it found, so a caller whose
reservation had been evicted freed the *current* owner's key and let a third
request execute the mutation a second time. `reserve` now takes a caller-minted
claim that `finalizeRecord` and `release` must present; a mismatched claim is a
no-op. The manager mints one per attempt.
**Every concurrent result (non-blocking).** Now collects all 16 outcomes and
asserts 1 `RESERVED`, 15 `DUPLICATE`, and no thrown exception, instead of
counting winners only.
**Controllable clock (non-blocking).** This one exposed the third bug:
`asMap().computeIfPresent` counts as a write under `expireAfterWrite`, so
finalizing restarted the timer and a record finalized at T+29m survived to
T+59m — past the lifetime advertised to clients. Rather than only testing it,
expiry now derives from each record's own `expiresAtMs`: `reserve` treats a
record past its deadline as absent, `load` filters it, and Caffeine's expiry is
demoted to memory reclamation. Behavior no longer depends on when cache
maintenance runs, and it matches how a database-backed store will compare
`expires_at`. The store takes an injected clock for tests.
One test changed premise as a result:
`testPurgeExpiredRemovesOnlyElapsedRecords` used to reserve an already-expired
record, which now never enters the cache at all, so it runs on the injected
clock instead.
`iceberg-common` is 101 tests green, and #12474 is rebased on this with the
manager updated for the new signatures (`iceberg-rest-server`, 419 tests green).
One follow-up worth flagging: this SPI change reaches the Phase 2 JDBC
store, which was written against the old three-argument `reserve`/`release`.
That work needs the claim plumbed through its schema too — a `claim` column on
`iceberg_idempotency_key_meta` and `AND claim = ?` on the finalize and release
statements — otherwise the JDBC store would silently lose the fencing guarantee
this review added.
--
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]
