yuqi1129 commented on PR #13606: URL: https://github.com/apache/gravitino/pull/13606#issuecomment-5907267291
Thanks for testing the head. Addressed in 4cb5c32b7d: **1. Concurrent-eviction test / false denial.** Confirmed, with two causes: - The post-reload check in b88c4cf44 read `loadedRoles` outside `rolePolicyLock` and failed closed for any bound role without a marker. Besides the race you describe, it denied deterministically whenever the user also held a partially loaded role (one referencing a dropped object has a backoff marker, never a loaded marker) or a dropped role. That check is replaced: `versionCheckAndLoadRoles` now returns the roles whose entity could not be read (`NoSuchEntityException` counts as dropped, not unreadable), and the check fails closed only when that set is non-empty. `testReloadReadFailureMustNotLoseDeny` still covers the lost-deny case; `testPartiallyLoadedRoleDoesNotFailClosedAfterEviction` covers the regression. - The test itself depended on timing: a 1 ms evictor against a bounded reload budget can legitimately exhaust it on a slow machine. The evictor now evicts at most once per request, at a random point in it, so the assertion is deterministic while the eviction still races the checks. Budget exhaustion is covered separately by `testRepeatedEvictionsFailClosed`. Passed 10/10 locally, 5 of them with all cores saturated. **2. Unbounded `roleClearGenerations`.** Agreed. The map is now capped at `roleCacheSize`: when it grows past that, the older half is pruned under the write lock and a `prunedRoleClearGeneration` watermark is raised. A request whose recorded generation is older than the watermark reloads its roles once (a no-op when nothing is missing), so pruning never hides a clear. Covered by `testRoleClearGenerationsArePrunedWithoutMissingClears`. **3. Duplicate reloads after one eviction.** The duplication is real, but it is not new: after an eviction, every new request's `loadRole` already reloads the role concurrently, since `versionCheckAndLoadRoles` has no single-flight. This PR extends the same path to in-flight requests, bounded by request concurrency, and resolution mostly hits `metadataIdCache` and the entity cache. It does not raise the fail-closed rate: a duplicate reload either finds the marker already restored or replaces the policies atomically inside `replaceRolePolicies`, and neither records a clear generation. I opened #13612 to add per-role single-flight to `versionCheckAndLoadRoles` for both paths. -- 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]
