You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "riemenschneider (via GitHub)" <gi...@apache.org> on 2026/10/13 11:52:32 UTC

[PR] NoSQL: Make the commit retry settings configurable [polaris]

riemenschneider opened a new pull request, #5779:
URL: https://github.com/apache/polaris/pull/5779

   NoSQL commits are retried within a total time budget (`RetryConfig.timeout`, default `PT15S`), but the retry settings could not be configured: `PersistenceParams.retryConfig()` was a `default` method, and SmallRye config mappings skip default methods.
   
   This change maps the existing `RetryConfig` as the `polaris.persistence.retry` group, which makes these properties available, with unchanged defaults:
   
   - `polaris.persistence.retry.timeout`
   - `polaris.persistence.retry.retries`
   - `polaris.persistence.retry.initial-sleep-lower`
   - `polaris.persistence.retry.initial-sleep-upper`
   - `polaris.persistence.retry.max-sleep`
   - `polaris.persistence.retry.fair-retries`
   
   `PersistenceParams.retryConfig()` is now abstract, and the now unused `RetryConfig.DEFAULT_RETRY_CONFIG` is removed. Both in-repo implementations already override `retryConfig()`; per the [evolution guidelines](https://github.com/apache/polaris/blob/main/site/content/in-dev/unreleased/evolution.md), Java classes may change at any time.
   
   Out of scope, to be handled separately:
   - aligning the documented MongoDB driver timeouts (`read-timeout`, `server-selection-timeout`: 30S) with the retry timeout;
   - a per-attempt deadline.
   
   Fixes #5777
   Discussed on dev@: https://lists.apache.org/thread/snms04dhn9d5g10g59nrfx6j2m1dw7ns
   
   I used AI assistance to implement this change; I have reviewed it.
   
   ## Checklist
   - [x] 🛡️ Don't disclose security issues! (contact security@apache.org)
   - [x] đź”— Clearly explained why the changes are needed, or linked related issues: Fixes #5777
   - [x] 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
   - [x] đź’ˇ Added comments for complex logic
   - [x] 🧾 Updated `CHANGELOG.md` (if needed)
   - [x] đź“š Updated documentation in `site/content/in-dev/unreleased` (if needed)
   
   🤖 Generated with [Claude Code](https://claude.com/claude-code)
   


-- 
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] NoSQL: Make the commit retry settings configurable [polaris]

Posted by "riemenschneider (via GitHub)" <gi...@apache.org>.
riemenschneider commented on code in PR #5779:
URL: https://github.com/apache/polaris/pull/5779#discussion_r4259535787


##########
persistence/nosql/persistence/api/src/test/java/org/apache/polaris/persistence/nosql/api/TestPersistenceParams.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.persistence.nosql.api;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import io.smallrye.config.PropertiesConfigSource;
+import io.smallrye.config.SmallRyeConfigBuilder;
+import java.time.Duration;
+import java.util.Map;
+import org.apache.polaris.persistence.nosql.api.commit.RetryConfig;
+import org.junit.jupiter.api.Test;
+
+public class TestPersistenceParams {
+  @Test
+  public void retryConfig() {
+    var params =
+        new SmallRyeConfigBuilder()
+            .withMapping(PersistenceParams.class)
+            .addDiscoveredConverters()
+            .withSources(
+                new PropertiesConfigSource(
+                    Map.of("polaris.persistence.retry.timeout", "PT45S"), "configMap"))
+            .build()
+            .getConfigMapping(PersistenceParams.class);
+
+    assertThat(params.retryConfig().timeout()).isEqualTo(Duration.ofSeconds(45));
+    assertThat(params.retryConfig().retries())
+        .isEqualTo(Integer.parseInt(RetryConfig.DEFAULT_RETRIES));

Review Comment:
   Thanks. The test now asserts the full mapped `RetryConfig` against the builder defaults (with the timeout override), which removes the parsing and covers all six properties.



-- 
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] NoSQL: Make the commit retry settings configurable [polaris]

Posted by "riemenschneider (via GitHub)" <gi...@apache.org>.
riemenschneider commented on code in PR #5779:
URL: https://github.com/apache/polaris/pull/5779#discussion_r4259540656


##########
site/content/in-dev/unreleased/configuration/config-sections/smallrye-polaris_persistence.md:
##########
@@ -31,11 +31,11 @@ build:
 | `polaris.persistence.max-index-stripes` | `20` | `int` |  |
 | `polaris.persistence.max-embedded-index-size` | `32k` | `MemorySize` |  |
 | `polaris.persistence.max-index-stripe-size` | `128k` | `MemorySize` |  |
-| `polaris.persistence.retry.timeout` | `PT15S` | `duration` | Maximum allowed time until a retry-loop and commits (Committer#commit(CommitRetryable)) fails with a (`RetryTimeoutException`), defaults to "PT15S".  |
-| `polaris.persistence.retry.retries` | `10000` | `int` | Maximum number of allowed retries, defaults to "10000".  |
-| `polaris.persistence.retry.initial-sleep-lower` | `PT0.010S` | `duration` | Initial lower bound for a retry-sleep duration for the retry-loop, defaults to (`PT0.010S`). This value will be doubled after each retry, as long as  (`#maxSleep()`) is not exceeded. A concrete sleep duration will be randomly chosen between  the current lower and upper bounds.  |
-| `polaris.persistence.retry.initial-sleep-upper` | `PT0.020S` | `duration` | Initial upper bound for a retry-sleep duration for the retry-loop, defaults to (`PT0.020S`). This value will be doubled after each retry, as long as  (`#maxSleep()`) is not exceeded. A concrete sleep duration will be randomly chosen between  the current lower and upper bounds.  |
-| `polaris.persistence.retry.max-sleep` | `PT0.250S` | `duration` | Maximum retry-sleep duration, defaults to (`PT0.250S`).  |
-| `polaris.persistence.retry.fair-retries` | `SLEEPING` | `enum (UNFAIR, SLEEPING)` | Without mitigation, very frequently started retry-loops running against highly contended  resources can result in some retry-loops invocations never making any progress and eventually  time out.  <br><br>The default "fair retries type" helps in these scenarios with sacrificing the overall  throughput too much.  |
-| `polaris.persistence.bucketized-bulk-fetch-size` | `16` | `int` | The number of objects to fetch at once via (`Persistence#bucketizedBulkFetches(Stream,<br> Class)`).  |
+| `polaris.persistence.retry.timeout` | `PT15S` | `duration` | Maximum time a retry loop allows `Committer#commit(CommitRetryable)` to keep retrying before failing with `RetryTimeoutException`. |

Review Comment:
   Argh, sorry! I just picked up Copilot's suggestion since it looks fine from a language perspective without checking that it is generated. I will revert the according commit.



-- 
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] NoSQL: Make the commit retry settings configurable [polaris]

Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on PR #5779:
URL: https://github.com/apache/polaris/pull/5779#issuecomment-6189562495

   Thanks @riemenschneider !


-- 
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] NoSQL: Make the commit retry settings configurable [polaris]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #5779:
URL: https://github.com/apache/polaris/pull/5779#discussion_r4259052953


##########
persistence/nosql/persistence/api/src/test/java/org/apache/polaris/persistence/nosql/api/TestPersistenceParams.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.persistence.nosql.api;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import io.smallrye.config.PropertiesConfigSource;
+import io.smallrye.config.SmallRyeConfigBuilder;
+import java.time.Duration;
+import java.util.Map;
+import org.apache.polaris.persistence.nosql.api.commit.RetryConfig;
+import org.junit.jupiter.api.Test;
+
+public class TestPersistenceParams {
+  @Test
+  public void retryConfig() {
+    var params =
+        new SmallRyeConfigBuilder()
+            .withMapping(PersistenceParams.class)
+            .addDiscoveredConverters()
+            .withSources(
+                new PropertiesConfigSource(
+                    Map.of("polaris.persistence.retry.timeout", "PT45S"), "configMap"))
+            .build()
+            .getConfigMapping(PersistenceParams.class);
+
+    assertThat(params.retryConfig().timeout()).isEqualTo(Duration.ofSeconds(45));
+    assertThat(params.retryConfig().retries())
+        .isEqualTo(Integer.parseInt(RetryConfig.DEFAULT_RETRIES));

Review Comment:
   Using `Integer.parseInt(RetryConfig.DEFAULT_RETRIES)` makes the test unnecessarily dependent on the default being stored as a `String`. Prefer asserting against a typed constant (if available) or the literal expected default (`10000`) to keep the test intent clear and avoid incidental parsing logic in the assertion.



##########
site/content/in-dev/unreleased/configuration/config-sections/smallrye-polaris_persistence.md:
##########
@@ -31,5 +31,11 @@ build:
 | `polaris.persistence.max-index-stripes` | `20` | `int` |  |
 | `polaris.persistence.max-embedded-index-size` | `32k` | `MemorySize` |  |
 | `polaris.persistence.max-index-stripe-size` | `128k` | `MemorySize` |  |
+| `polaris.persistence.retry.timeout` | `PT15S` | `duration` | Maximum allowed time until a retry-loop and commits (Committer#commit(CommitRetryable)) fails with a (`RetryTimeoutException`), defaults to "PT15S".  |
+| `polaris.persistence.retry.retries` | `10000` | `int` | Maximum number of allowed retries, defaults to "10000".  |
+| `polaris.persistence.retry.initial-sleep-lower` | `PT0.010S` | `duration` | Initial lower bound for a retry-sleep duration for the retry-loop, defaults to (`PT0.010S`). This value will be doubled after each retry, as long as  (`#maxSleep()`) is not exceeded. A concrete sleep duration will be randomly chosen between  the current lower and upper bounds.  |
+| `polaris.persistence.retry.initial-sleep-upper` | `PT0.020S` | `duration` | Initial upper bound for a retry-sleep duration for the retry-loop, defaults to (`PT0.020S`). This value will be doubled after each retry, as long as  (`#maxSleep()`) is not exceeded. A concrete sleep duration will be randomly chosen between  the current lower and upper bounds.  |
+| `polaris.persistence.retry.max-sleep` | `PT0.250S` | `duration` | Maximum retry-sleep duration, defaults to (`PT0.250S`).  |
+| `polaris.persistence.retry.fair-retries` | `SLEEPING` | `enum (UNFAIR, SLEEPING)` | Without mitigation, very frequently started retry-loops running against highly contended  resources can result in some retry-loops invocations never making any progress and eventually  time out.  <br><br>The default "fair retries type" helps in these scenarios with sacrificing the overall  throughput too much.  |
 | `polaris.persistence.bucketized-bulk-fetch-size` | `16` | `int` | The number of objects to fetch at once via (`Persistence#bucketizedBulkFetches(Stream,<br> Class)`).  |

Review Comment:
   The new config table entries have several clarity/grammar issues and redundant phrasing: (1) descriptions repeatedly say “defaults to …” even though the default value is already a dedicated column; (2) “until a retry-loop and commits … fails” is grammatically incorrect and reads ambiguously; (3) `(`#maxSleep()` )` is unclear without a type qualifier (e.g., `RetryConfig#maxSleep()`), and `a (`RetryTimeoutException`)` has extra punctuation; (4) there are multiple double-spaces and the phrase “retry-loops invocations” should be “retry-loop invocations”, and “helps … with sacrificing” should be “helps … while sacrificing”. Suggest rewriting these descriptions to be concise, remove redundant default mentions, and use consistent API references.



-- 
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] NoSQL: Make the commit retry settings configurable [polaris]

Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #5779:
URL: https://github.com/apache/polaris/pull/5779#discussion_r4259510147


##########
site/content/in-dev/unreleased/configuration/config-sections/smallrye-polaris_persistence.md:
##########
@@ -31,11 +31,11 @@ build:
 | `polaris.persistence.max-index-stripes` | `20` | `int` |  |
 | `polaris.persistence.max-embedded-index-size` | `32k` | `MemorySize` |  |
 | `polaris.persistence.max-index-stripe-size` | `128k` | `MemorySize` |  |
-| `polaris.persistence.retry.timeout` | `PT15S` | `duration` | Maximum allowed time until a retry-loop and commits (Committer#commit(CommitRetryable)) fails with a (`RetryTimeoutException`), defaults to "PT15S".  |
-| `polaris.persistence.retry.retries` | `10000` | `int` | Maximum number of allowed retries, defaults to "10000".  |
-| `polaris.persistence.retry.initial-sleep-lower` | `PT0.010S` | `duration` | Initial lower bound for a retry-sleep duration for the retry-loop, defaults to (`PT0.010S`). This value will be doubled after each retry, as long as  (`#maxSleep()`) is not exceeded. A concrete sleep duration will be randomly chosen between  the current lower and upper bounds.  |
-| `polaris.persistence.retry.initial-sleep-upper` | `PT0.020S` | `duration` | Initial upper bound for a retry-sleep duration for the retry-loop, defaults to (`PT0.020S`). This value will be doubled after each retry, as long as  (`#maxSleep()`) is not exceeded. A concrete sleep duration will be randomly chosen between  the current lower and upper bounds.  |
-| `polaris.persistence.retry.max-sleep` | `PT0.250S` | `duration` | Maximum retry-sleep duration, defaults to (`PT0.250S`).  |
-| `polaris.persistence.retry.fair-retries` | `SLEEPING` | `enum (UNFAIR, SLEEPING)` | Without mitigation, very frequently started retry-loops running against highly contended  resources can result in some retry-loops invocations never making any progress and eventually  time out.  <br><br>The default "fair retries type" helps in these scenarios with sacrificing the overall  throughput too much.  |
-| `polaris.persistence.bucketized-bulk-fetch-size` | `16` | `int` | The number of objects to fetch at once via (`Persistence#bucketizedBulkFetches(Stream,<br> Class)`).  |
+| `polaris.persistence.retry.timeout` | `PT15S` | `duration` | Maximum time a retry loop allows `Committer#commit(CommitRetryable)` to keep retrying before failing with `RetryTimeoutException`. |

Review Comment:
   These manual changes won't work, because these files are generated from the Javadocs.



-- 
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] NoSQL: Make the commit retry settings configurable [polaris]

Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy merged PR #5779:
URL: https://github.com/apache/polaris/pull/5779


-- 
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