You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "huaxingao (via GitHub)" <gi...@apache.org> on 2026/07/21 18:52:48 UTC
[PR] Scale idempotency key-window cap with TTL and harden window decoding [polaris]
huaxingao opened a new pull request, #5122:
URL: https://github.com/apache/polaris/pull/5122
<!--
๐ Describe what changes you're proposing, especially breaking or user-facing changes.
๐ See https://github.com/apache/polaris/blob/main/CONTRIBUTING.md for more.
-->
Follow-up polish to the entity-property idempotency feature:
1. **TTL-scaled key-window cap.** The per-entity live-key window was capped at a
hardcoded `MAX_WINDOW_SIZE = 64` regardless of the configured key TTL. The cap
now scales linearly from that baseline (64 keys at the 5-minute default TTL, e.g.
128 at 10m, 384 at 30m), floored at 1 and clamped at 1024 so a large TTL can't
grow the per-entity property without bound.
2. **Graceful decode.** A corrupt or unrecognized key window (unknown format
prefix, invalid base64, or unreadable SMILE) is now treated as "no live keys"
instead of throwing. This keeps a malformed property from failing the whole
create/update; the next write overwrites it with a valid window (self-healing).
3. **Rename.** Private constant `WINDOW_FORMAT_SMILE_V1` -> `FORMAT_SMILE_V1`
(wire value `IS1` unchanged).
## Checklist
- [ ] ๐ก๏ธ Don't disclose security issues! (contact security@apache.org)
- [ ] ๐ Clearly explained why the changes are needed, or linked related issues: Fixes #
- [ ] ๐งช Added/updated tests with good coverage, or manually tested (and explained how)
- [ ] ๐ก Added comments for complex logic
- [ ] ๐งพ Updated `CHANGELOG.md` (if needed)
- [ ] ๐ Updated documentation in `site/content/in-dev/unreleased` (if needed)
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Follow-up polish to the `createTable`/`updateTable` idempotency work. [polaris]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #5122:
URL: https://github.com/apache/polaris/pull/5122#discussion_r3642013820
##########
runtime/service/src/main/java/org/apache/polaris/service/idempotency/EntityIdempotency.java:
##########
@@ -130,24 +126,24 @@ 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) {
+ return List.of();
Review Comment:
Sounds good! Will add one.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Follow-up polish to the `createTable`/`updateTable` idempotency work. [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain merged PR #5122:
URL: https://github.com/apache/polaris/pull/5122
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Scale idempotency key-window cap with TTL and harden window decoding [polaris]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #5122:
URL: https://github.com/apache/polaris/pull/5122#issuecomment-5051864693
Per an offline discussion with @flyrain, I dropped the count-based window cap (`MAX_WINDOW_SIZE`) entirely rather than scaling it with the TTL. A fixed count can evict a still-live key, which silently disables idempotency for that key and reintroduces the exact corruption failure mode this feature prevents. We'd rather accept a larger per-entity window under a high write rate than ever drop a key that could still be retried. The window is now bounded solely by expiry (TTL).
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Follow-up polish to the `createTable`/`updateTable` idempotency work. [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5122:
URL: https://github.com/apache/polaris/pull/5122#discussion_r3647374339
##########
runtime/service/src/main/java/org/apache/polaris/service/idempotency/EntityIdempotency.java:
##########
@@ -130,24 +126,24 @@ 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) {
+ return List.of();
}
}
private static String encode(Collection<KeyEntry> window) {
try {
byte[] smile = SMILE_MAPPER.writeValueAsBytes(window);
- return WINDOW_FORMAT_SMILE_V1 + Base64.getUrlEncoder().withoutPadding().encodeToString(smile);
+ return FORMAT_SMILE_V1 + Base64.getUrlEncoder().withoutPadding().encodeToString(smile);
Review Comment:
Yes, failing the most recent request that overflows the UUID "window" sounds preferable to silintly ignoring unexpired idempotency keys.
The question is which error code should be use?.. I think 503 would be appropriate. WDYT?
Cf. #5144
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Follow-up polish to the `createTable`/`updateTable` idempotency work. [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #5122:
URL: https://github.com/apache/polaris/pull/5122#issuecomment-5052029409
> A fixed count can evict a still-live key, which silently disables idempotency for that key and reintroduces the exact corruption failure mode this feature prevents.
Thanks @huaxingao for the fix. It's critical that we never drop a valid idempotency key. Otherwise, a client could retry an operation that should have been recognized as a duplicate, potentially leading to table corruption.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Follow-up polish to the `createTable`/`updateTable` idempotency work. [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5122:
URL: https://github.com/apache/polaris/pull/5122#discussion_r3641928919
##########
runtime/service/src/main/java/org/apache/polaris/service/idempotency/EntityIdempotency.java:
##########
@@ -130,24 +126,24 @@ 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) {
+ return List.of();
Review Comment:
Is it worth an ERROR log message?
##########
runtime/service/src/main/java/org/apache/polaris/service/idempotency/EntityIdempotency.java:
##########
@@ -130,24 +126,24 @@ 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) {
+ return List.of();
}
}
private static String encode(Collection<KeyEntry> window) {
try {
byte[] smile = SMILE_MAPPER.writeValueAsBytes(window);
- return WINDOW_FORMAT_SMILE_V1 + Base64.getUrlEncoder().withoutPadding().encodeToString(smile);
+ return FORMAT_SMILE_V1 + Base64.getUrlEncoder().withoutPadding().encodeToString(smile);
Review Comment:
With `MAX_WINDOW_SIZE` gone, we should probably have a size-based guardrail (possible with a large default value).... Otherwise the server will become vulnerable to DOS attacks via idempotency data overload.
##########
runtime/service/src/main/java/org/apache/polaris/service/idempotency/EntityIdempotency.java:
##########
@@ -130,24 +126,24 @@ 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) {
+ return List.of();
}
}
private static String encode(Collection<KeyEntry> window) {
try {
byte[] smile = SMILE_MAPPER.writeValueAsBytes(window);
- return WINDOW_FORMAT_SMILE_V1 + Base64.getUrlEncoder().withoutPadding().encodeToString(smile);
+ return FORMAT_SMILE_V1 + Base64.getUrlEncoder().withoutPadding().encodeToString(smile);
Review Comment:
With `MAX_WINDOW_SIZE` gone, we should probably have a size-based guardrail (possibly with a large default value).... Otherwise the server will become vulnerable to DOS attacks via idempotency data overload.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Follow-up polish to the `createTable`/`updateTable` idempotency work. [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5122:
URL: https://github.com/apache/polaris/pull/5122#discussion_r3653121897
##########
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:
JB, good questions. 10K is the upper bound to mitigate DoS per discussion with @dimas-b. The main knob to control the volume is the TTL. The default setting is 5 mins. In that case, the number of key-value pairs is under 60 when a high commit frequency like every 5 seconds happens. @huaxingao also did a bunch of benchmark in this PR to cover more values, https://github.com/apache/polaris/pull/4912#issuecomment-4814094846. Please check it out.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Follow-up polish to the `createTable`/`updateTable` idempotency work. [polaris]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Follow-up polish to the `createTable`/`updateTable` idempotency work. [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #5122:
URL: https://github.com/apache/polaris/pull/5122#discussion_r3653121897
##########
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:
JB, good questions. 10K is up boundary to avoid DoS per discussion with @dimas-b. The main knob to control the volume is the TTL.
@huaxingao also did a bunch of benchmark in this PR, https://github.com/apache/polaris/pull/4912#issuecomment-4814094846. Please check it out.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Follow-up polish to the `createTable`/`updateTable` idempotency work. [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #5122:
URL: https://github.com/apache/polaris/pull/5122#issuecomment-5095612245
Thanks @jbonofre ! Checked with @dimas-b offline, he is OK to merge as well. I will merge this to unblock the release.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Follow-up polish to the `createTable`/`updateTable` idempotency work. [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5122:
URL: https://github.com/apache/polaris/pull/5122#discussion_r3641926551
##########
runtime/service/src/main/java/org/apache/polaris/service/idempotency/EntityIdempotency.java:
##########
@@ -130,24 +126,24 @@ 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) {
+ return List.of();
}
}
private static String encode(Collection<KeyEntry> window) {
try {
byte[] smile = SMILE_MAPPER.writeValueAsBytes(window);
- return WINDOW_FORMAT_SMILE_V1 + Base64.getUrlEncoder().withoutPadding().encodeToString(smile);
+ return FORMAT_SMILE_V1 + Base64.getUrlEncoder().withoutPadding().encodeToString(smile);
Review Comment:
With `MAX_WINDOW_SIZE` gone, we should probably have a size-based guardrail (possibly with a large default value).... Otherwise the server will become vulnerable to DoS attacks via idempotency data overload.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Follow-up polish to the `createTable`/`updateTable` idempotency work. [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5122:
URL: https://github.com/apache/polaris/pull/5122#discussion_r3647374339
##########
runtime/service/src/main/java/org/apache/polaris/service/idempotency/EntityIdempotency.java:
##########
@@ -130,24 +126,24 @@ 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) {
+ return List.of();
}
}
private static String encode(Collection<KeyEntry> window) {
try {
byte[] smile = SMILE_MAPPER.writeValueAsBytes(window);
- return WINDOW_FORMAT_SMILE_V1 + Base64.getUrlEncoder().withoutPadding().encodeToString(smile);
+ return FORMAT_SMILE_V1 + Base64.getUrlEncoder().withoutPadding().encodeToString(smile);
Review Comment:
Yes, failing the most recent request that overflows the UUID "window" sounds preferable to silently ignoring unexpired idempotency keys.
The question is which error code should be use?.. I think 503 would be appropriate. WDYT?
Cf. #5144
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Follow-up polish to the `createTable`/`updateTable` idempotency work. [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Follow-up polish to the `createTable`/`updateTable` idempotency work. [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #5122:
URL: https://github.com/apache/polaris/pull/5122#discussion_r3648549080
##########
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:
This is not a blocker either. The feature is still in "beta" and can be disabled as a whole.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Scale idempotency key-window cap with TTL and harden window decoding [polaris]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #5122:
URL: https://github.com/apache/polaris/pull/5122#discussion_r3624906865
##########
runtime/service/src/main/java/org/apache/polaris/service/idempotency/EntityIdempotency.java:
##########
@@ -110,10 +120,11 @@ 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) {
+ // Bounded window, sized from this key's TTL. Drop the earliest-expiring entries (front of the
+ // sorted list) *before* inserting so the key just recorded is always retained. This matters
+ // when entries share an expiry (e.g. a burst of writes) and expiry order can't rank recency.
+ int maxWindowSize = maxWindowSize(Duration.between(now, expiry));
+ while (window.size() >= maxWindowSize) {
Review Comment:
maxWindowSize is currently computed from `Duration.between(now, expiry)`, which is the *remaining* time-to-expiry at the moment `recordKey` is called. In the main call site (`IdempotencyRequestContext` computes `pendingExpiry` up-front, but `LocalIcebergCatalog` passes a later `Instant.now()`), the effective cap will shrink as the request runs (e.g., a 5m configured TTL becomes a 1m remaining TTL -> cap ~12), causing premature eviction and contradicting the intent of scaling with the configured TTL.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Scale idempotency key-window cap with TTL and harden window decoding [polaris]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #5122:
URL: https://github.com/apache/polaris/pull/5122#issuecomment-5048957585
@dimas-b @flyrain Could you please take a look when you have a moment? Thanks a lot!
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Follow-up polish to the `createTable`/`updateTable` idempotency work. [polaris]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #5122:
URL: https://github.com/apache/polaris/pull/5122#discussion_r3642012505
##########
runtime/service/src/main/java/org/apache/polaris/service/idempotency/EntityIdempotency.java:
##########
@@ -130,24 +126,24 @@ 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) {
+ return List.of();
}
}
private static String encode(Collection<KeyEntry> window) {
try {
byte[] smile = SMILE_MAPPER.writeValueAsBytes(window);
- return WINDOW_FORMAT_SMILE_V1 + Base64.getUrlEncoder().withoutPadding().encodeToString(smile);
+ return FORMAT_SMILE_V1 + Base64.getUrlEncoder().withoutPadding().encodeToString(smile);
Review Comment:
Thanks for the catch, I completely agree we need a guardrail here. Would it work for you if I set a very large cap and, when it's exceeded, fail the write rather than evict? My only hesitation with eviction is that it could drop a still-live key, which @flyrain flagged as a possible corruption risk.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Follow-up polish to the `createTable`/`updateTable` idempotency work. [polaris]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #5122:
URL: https://github.com/apache/polaris/pull/5122#discussion_r3650571290
##########
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:
Changed to `PolarisServiceUnavailableException`
##########
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:
Since this is still in "beta" and can be disabled as a whole, and 10,000 is far above any real workload, I'd rather keep it a constant for now and make it configurable later if someone actually needs it.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org