dimas-b commented on code in PR #5122:
URL: https://github.com/apache/polaris/pull/5122#discussion_r3648493561
##########
runtime/service/src/main/java/org/apache/polaris/service/idempotency/EntityIdempotency.java:
##########
@@ -110,11 +149,14 @@ public static Map<String, String> recordKey(
}
}
- // Bounded window: drop the earliest-expiring entries (front of the sorted
list) to make room
- // *before* inserting, so the key just recorded is always retained even
when every entry shares
- // the same expiry (e.g. a burst of writes at one instant) and expiry
order can't rank recency.
- while (window.size() >= MAX_WINDOW_SIZE) {
- window.remove(0);
+ // Fail rather than evict a live key: dropping one would silently disable
idempotency for it.
+ // 503 (retryable) is apt: once some of the live keys expire the write can
succeed.
+ if (window.size() >= maxLiveKeys) {
+ throw new ServiceUnavailableException(
Review Comment:
I believe `PolarisServiceUnavailableException` is preferable because it
informs clients that a retry is possible.
Not a blocker, though.
##########
runtime/service/src/main/java/org/apache/polaris/service/idempotency/EntityIdempotency.java:
##########
@@ -52,20 +56,32 @@
* evolve the encoding later without ambiguity.
*
* <p>For {@code createTable} this window holds a single entry, but the shape
generalizes to a
- * bounded per-entity window for repeated mutations (e.g. {@code
updateTable}). Expired keys are
- * dropped inline whenever the entity is rewritten ({@link #recordKey});
expiry is also honored at
- * read time ({@link #hasLiveKey}) so a stale-but-not-yet-purged key is
treated as absent.
+ * per-entity window for repeated mutations (e.g. {@code updateTable}). The
window is bounded only
+ * by expiry (TTL): expired keys are dropped inline whenever the entity is
rewritten ({@link
+ * #recordKey}), and expiry is also honored at read time ({@link #hasLiveKey})
so a
+ * stale-but-not-yet-purged key is treated as absent. Live keys are never
evicted by count; a large
+ * safety ceiling ({@link #MAX_LIVE_KEYS}) bounds the window by failing the
write rather than
+ * dropping a still-live key.
*/
public final class EntityIdempotency {
+ private static final Logger LOGGER =
LoggerFactory.getLogger(EntityIdempotency.class);
+
/** Internal-properties key under which the per-entity idempotency key
window is stored. */
public static final String IDEMPOTENCY_KEYS_PROPERTY =
"polaris-idempotency-keys";
- /** Upper bound on live idempotency keys stored on a single entity. */
- public static final int MAX_WINDOW_SIZE = 64;
-
/** Magic prefix for the version 1 SMILE window ({@code IS1} + base64url
bytes). */
- private static final String WINDOW_FORMAT_SMILE_V1 = "IS1";
+ private static final String FORMAT_SMILE_V1 = "IS1";
+
+ /**
+ * Safety ceiling on the number of live keys retained on a single entity.
Unlike a count-based
+ * eviction cap, this never drops a live key: when the window is full the
write fails instead, so
+ * a key that could still be retried is never silently discarded. The limit
is deliberately high —
+ * reaching it requires this many successful commits to one table within the
key TTL — so
+ * legitimate workloads never hit it, while still bounding stored
idempotency data against
+ * unbounded growth (a DoS guardrail).
+ */
+ private static final int MAX_LIVE_KEYS = 10_000;
Review Comment:
Should this be user-configurable?
--
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]
