jbonofre commented on code in PR #5122:
URL: https://github.com/apache/polaris/pull/5122#discussion_r3653008831
##########
runtime/service/src/main/java/org/apache/polaris/service/idempotency/EntityIdempotency.java:
##########
@@ -130,24 +175,31 @@ public static Map<String, String> recordKey(
}
private static List<KeyEntry> decode(String raw) {
- if (raw == null || raw.isEmpty()) {
+ if (raw == null || !raw.startsWith(FORMAT_SMILE_V1)) {
return List.of();
}
- if (!raw.startsWith(WINDOW_FORMAT_SMILE_V1)) {
- throw new IllegalArgumentException("Unrecognized idempotency key window
format");
- }
- byte[] smile =
Base64.getUrlDecoder().decode(raw.substring(WINDOW_FORMAT_SMILE_V1.length()));
+ // An unreadable window (bad base64 or SMILE decode failure) is treated as
no live keys rather
+ // than failing the operation: a corrupt property degrades to normal
behavior and the next
+ // recordKey overwrites it.
try {
+ byte[] smile =
Base64.getUrlDecoder().decode(raw.substring(FORMAT_SMILE_V1.length()));
return SMILE_MAPPER.readValue(smile, new TypeReference<List<KeyEntry>>()
{});
- } catch (IOException e) {
- throw new RuntimeException("Failed to decode idempotency key window", e);
+ } catch (IllegalArgumentException | IOException e) {
+ // Server-written data, so a decode failure means corruption or a
tampered property; surface
Review Comment:
nit: the code does not surface it to the caller (it swallows and logs WARN).
I read "Surface it" as "throw". I just suggest to reword from "surface it" to
"log it".
##########
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:
I have a question about this value (10,000) vs the storage limits.
If we have 10,000 UUID entries (encoded with base64), it means about several
hundred KB stored in a single internal-property value. Depending on the
persistence backend's column/document size limit, a write could fail at the
storage layer before the 10,000 guardrail (as a non-retryable error, not the
graceful 503 error code intends).
I think it would be good to double check the guardrail sits below any real
storage ceiling, or a least noting the assumption.
--
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]
