You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "andrew4699 (via GitHub)" <gi...@apache.org> on 2025/02/21 22:17:51 UTC
[PR] More sensible BasePolarisCatalog retry behavior [polaris]
andrew4699 opened a new pull request, #1046:
URL: https://github.com/apache/polaris/pull/1046
<!--
Possible security vulnerabilities: STOP here and contact security@apache.org instead!
Please update the title of the PR with a meaningful message - do not leave it "empty" or "generated"
Please update this summary field:
The summary should cover these topics, if applicable:
* the motivation for the change
* a description of the status quo, for example the current behavior
* the desired behavior
* etc
PR checklist:
- Do a self-review of your code before opening a pull request
- Make sure that there's good test coverage for the changes included in this PR
- Run tests locally before pushing a PR (./gradlew check)
- Code should have comments where applicable. Particularly hard-to-understand
areas deserve good in-line documentation.
- Include changes and enhancements to the documentation (in site/content/in-dev/unreleased)
- For Work In Progress Pull Requests, please use the Draft PR feature.
Make sure to add the information BELOW this comment.
Everything in this comment will NOT be added to the PR description.
-->
Previously BasePolarisCatalog had very aggressive retries. It would do up to 12 retries and would retry unauthorized, not found, and other typically non-retryable errors.
This PR makes the retry behavior more sensible. I added a config for the max number of retries, with a default value of 12 for backward compatibility. This is an improvement over the hard-coded value of 12. I also restricted the retries to timeouts, rate limits, 500, 503, and unknown errors.
--
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] More sensible BasePolarisCatalog retry behavior [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1046:
URL: https://github.com/apache/polaris/pull/1046#discussion_r1966307487
##########
service/common/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java:
##########
@@ -2014,6 +2025,12 @@ private Boolean getBooleanContextConfiguration(String configKey, boolean default
* @return true if the exception is retryable
*/
private static boolean isStorageProviderRetryableException(Exception ex) {
+ if (ex instanceof RuntimeException rex
+ && !RETRYABLE_CLOUD_EXCEPTION_CODES.contains(
+ IcebergExceptionMapper.extractHttpCodeFromCloudException(rex))) {
Review Comment:
We use `SdkException.retryable()` below, but why would we want to override what AWS SDK considers retryable here?
--
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] More sensible BasePolarisCatalog retry behavior [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1046:
URL: https://github.com/apache/polaris/pull/1046#discussion_r1972507075
##########
quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/BasePolarisCatalogTest.java:
##########
@@ -1521,23 +1530,46 @@ private void createNonExistingNamespaces(Namespace namespace) {
}
}
- @Test
- public void testRetriableException() {
- Iterator<String> accessDeniedHint =
- Iterators.cycle(IcebergExceptionMapper.getAccessDeniedHints());
- RuntimeException s3Exception = new RuntimeException(accessDeniedHint.next());
- RuntimeException azureBlobStorageException = new RuntimeException(accessDeniedHint.next());
- RuntimeException gcsException = new RuntimeException(accessDeniedHint.next());
- RuntimeException otherException = new RuntimeException(new IOException("Connection reset"));
- Assertions.assertThat(BasePolarisCatalog.SHOULD_RETRY_REFRESH_PREDICATE.test(s3Exception))
- .isFalse();
- Assertions.assertThat(
- BasePolarisCatalog.SHOULD_RETRY_REFRESH_PREDICATE.test(azureBlobStorageException))
- .isFalse();
- Assertions.assertThat(BasePolarisCatalog.SHOULD_RETRY_REFRESH_PREDICATE.test(gcsException))
- .isFalse();
- Assertions.assertThat(BasePolarisCatalog.SHOULD_RETRY_REFRESH_PREDICATE.test(otherException))
- .isTrue();
+ @ParameterizedTest
+ @MethodSource
+ public void testRetriableException(Exception exception, boolean shouldRetry) {
+ Assertions.assertThat(BasePolarisCatalog.SHOULD_RETRY_REFRESH_PREDICATE.test(exception))
+ .isEqualTo(shouldRetry);
+ }
+
+ static Stream<Arguments> testRetriableException() {
+ Set<Integer> NON_RETRYABLE_CODES = Set.of(401, 403, 404);
+ Set<Integer> RETRYABLE_CODES = Set.of(408, 504);
+
+ // Create a map of HTTP code returned from a cloud provider to whether it should be retried
+ Map<Integer, Boolean> cloudCodeMappings = new HashMap<>();
+ NON_RETRYABLE_CODES.forEach(code -> cloudCodeMappings.put(code, false));
+ RETRYABLE_CODES.forEach(code -> cloudCodeMappings.put(code, true));
+
+ return Stream.of(
+ Stream.of(
+ Arguments.of(new RuntimeException(new IOException("Connection reset")), true),
+ Arguments.of(RetryableException.builder().build(), true),
+ Arguments.of(NonRetryableException.builder().build(), false)),
+ IcebergExceptionMapper.getAccessDeniedHints().stream()
+ .map(hint -> Arguments.of(new RuntimeException(hint), false)),
+ cloudCodeMappings.entrySet().stream()
+ .flatMap(
+ entry ->
+ Stream.of(
+ Arguments.of(
+ new HttpResponseException(
+ "", new FakeAzureHttpResponse(entry.getKey()), ""),
+ entry.getValue()),
+ Arguments.of(
+ new StorageException(entry.getKey(), ""), entry.getValue()))),
+ IcebergExceptionMapper.RETRYABLE_AZURE_HTTP_CODES.stream()
+ .map(
+ code ->
+ Arguments.of(
+ new HttpResponseException("", new FakeAzureHttpResponse(code), ""),
+ true)))
+ .flatMap(Function.identity());
Review Comment:
nit: I believe there's a "concat" method for Streams.
--
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] More sensible BasePolarisCatalog retry behavior [polaris]
Posted by "andrew4699 (via GitHub)" <gi...@apache.org>.
andrew4699 commented on code in PR #1046:
URL: https://github.com/apache/polaris/pull/1046#discussion_r1972209286
##########
polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java:
##########
@@ -264,4 +264,13 @@ public static <T> Builder<T> builder() {
+ STORAGE_CREDENTIAL_DURATION_SECONDS.key)
.defaultValue(30 * 60) // 30 minutes
.build();
+
+ // The default value is kept at 12 for backward compatibility
+ public static final PolarisConfiguration<Integer> MAX_METADATA_REFRESH_RETRIES =
+ PolarisConfiguration.<Integer>builder()
+ .key("MAX_METADATA_REFRESH_RETRIES")
+ .description(
+ "How many times to retry refreshing metadata when the previous error was retryable")
+ .defaultValue(12)
+ .build();
Review Comment:
Updated it to 2!
--
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] More sensible BasePolarisCatalog retry behavior [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #1046:
URL: https://github.com/apache/polaris/pull/1046#discussion_r1972287077
##########
service/common/src/main/java/org/apache/polaris/service/exception/IcebergExceptionMapper.java:
##########
@@ -58,10 +59,24 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.slf4j.event.Level;
+import software.amazon.awssdk.core.exception.SdkException;
import software.amazon.awssdk.services.s3.model.S3Exception;
@Provider
public class IcebergExceptionMapper implements ExceptionMapper<RuntimeException> {
+ /** Signifies that we could not extract an HTTP code from a given cloud exception */
+ public static final int UNKNOWN_CLOUD_HTTP_CODE = -1;
+
+ @VisibleForTesting
Review Comment:
Nit: we could remove this tag as it needs to be public for both non-test code and test code.
##########
service/common/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java:
##########
@@ -2104,37 +2103,12 @@ private Boolean getBooleanContextConfiguration(String configKey, boolean default
.getConfiguration(callContext.getPolarisCallContext(), configKey, defaultValue);
}
- /**
- * Check if the exception is retryable for the storage provider
- *
- * @param ex exception
- * @return true if the exception is retryable
- */
- private static boolean isStorageProviderRetryableException(Exception ex) {
- // For S3/Azure, the exception is not wrapped, while for GCP the exception is wrapped as a
- // RuntimeException
- Throwable rootCause = ExceptionUtils.getRootCause(ex);
- if (rootCause == null) {
- // no root cause, let it retry
- return true;
- }
- // only S3 SdkException has this retryable property
- if (rootCause instanceof SdkException && ((SdkException) rootCause).retryable()) {
- return true;
- }
- // add more cases here if needed
- // AccessDenied is not retryable
- return !isAccessDenied(rootCause.getMessage());
- }
-
- private static boolean isAccessDenied(String errorMsg) {
- // Corresponding error messages for storage providers Aws/Azure/Gcp
- boolean isAccessDenied =
- errorMsg != null && IcebergExceptionMapper.containsAnyAccessDeniedHint(errorMsg);
- if (isAccessDenied) {
- LOGGER.debug("Access Denied or Forbidden error: {}", errorMsg);
- return true;
- }
- return false;
+ /** Helper to retrieve the max number of metadata refresh retries */
Review Comment:
Nit: I guess this comment isn't necessary as the method name is descriptive enough.
--
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] More sensible BasePolarisCatalog retry behavior [polaris]
Posted by "andrew4699 (via GitHub)" <gi...@apache.org>.
andrew4699 commented on code in PR #1046:
URL: https://github.com/apache/polaris/pull/1046#discussion_r1972115053
##########
polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java:
##########
@@ -264,4 +264,13 @@ public static <T> Builder<T> builder() {
+ STORAGE_CREDENTIAL_DURATION_SECONDS.key)
.defaultValue(30 * 60) // 30 minutes
.build();
+
+ // The default value is kept at 12 for backward compatibility
+ public static final PolarisConfiguration<Integer> MAX_METADATA_REFRESH_RETRIES =
+ PolarisConfiguration.<Integer>builder()
+ .key("MAX_METADATA_REFRESH_RETRIES")
+ .description(
+ "How many times to retry refreshing metadata when the previous error was retryable")
+ .defaultValue(12)
+ .build();
Review Comment:
> current deployment config
Curious what you mean by this. Do you mean setting this in one of the k8s/application properties files?
I'd be very much in favor of lowering the default as 12 is quite large and potentially dangerous. I'd be surprised if anyone is getting benefit from it being so high.
--
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] More sensible BasePolarisCatalog retry behavior [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #1046:
URL: https://github.com/apache/polaris/pull/1046#discussion_r1972139812
##########
polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java:
##########
@@ -264,4 +264,13 @@ public static <T> Builder<T> builder() {
+ STORAGE_CREDENTIAL_DURATION_SECONDS.key)
.defaultValue(30 * 60) // 30 minutes
.build();
+
+ // The default value is kept at 12 for backward compatibility
+ public static final PolarisConfiguration<Integer> MAX_METADATA_REFRESH_RETRIES =
+ PolarisConfiguration.<Integer>builder()
+ .key("MAX_METADATA_REFRESH_RETRIES")
+ .description(
+ "How many times to retry refreshing metadata when the previous error was retryable")
+ .defaultValue(12)
+ .build();
Review Comment:
> Do you mean setting this in one of the k8s/application properties files?
I was talking about the Snowfalke's Polaris deployment.
> I'd be surprised if anyone is getting benefit from it being so high.
I believe not. The only compatibility concern is the current production deployment, including Snowflake, Zsaler, etc. Given that we don't have a release yet, the impact will manageable. In general, it should be fine to prefer a reasonable retry number(2 or 3) to compatibility. cc @MonkeyCanCode
--
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] More sensible BasePolarisCatalog retry behavior [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #1046:
URL: https://github.com/apache/polaris/pull/1046#discussion_r1968291406
##########
polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java:
##########
@@ -264,4 +264,13 @@ public static <T> Builder<T> builder() {
+ STORAGE_CREDENTIAL_DURATION_SECONDS.key)
.defaultValue(30 * 60) // 30 minutes
.build();
+
+ // The default value is kept at 12 for backward compatibility
+ public static final PolarisConfiguration<Integer> MAX_METADATA_REFRESH_RETRIES =
+ PolarisConfiguration.<Integer>builder()
+ .key("MAX_METADATA_REFRESH_RETRIES")
+ .description(
+ "How many times to retry refreshing metadata when the previous error was retryable")
+ .defaultValue(12)
+ .build();
Review Comment:
I agree with the backward compatibility,
[doubt] These retries are on top of SDK retries ?
why are we did we go with a such high number ? default for hive catalog impl is 2 more details
: https://github.com/apache/iceberg/pull/5510#issuecomment-1226712167
--
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] More sensible BasePolarisCatalog retry behavior [polaris]
Posted by "andrew4699 (via GitHub)" <gi...@apache.org>.
andrew4699 commented on code in PR #1046:
URL: https://github.com/apache/polaris/pull/1046#discussion_r1972111920
##########
service/common/src/main/java/org/apache/polaris/service/exception/IcebergExceptionMapper.java:
##########
@@ -158,18 +161,25 @@ static int mapExceptionToResponseCode(RuntimeException rex) {
};
}
+ /**
+ * @return UNKNOWN_CLOUD_HTTP_CODE if the exception is not a cloud exception that we know how to
+ * extract the code from
+ */
Review Comment:
Updated!
##########
service/common/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java:
##########
@@ -1484,7 +1501,12 @@ public void doRefresh() {
refreshFromMetadataLocation(
latestLocation,
SHOULD_RETRY_REFRESH_PREDICATE,
- MAX_RETRIES,
+ callContext
Review Comment:
Updated!
##########
service/common/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java:
##########
@@ -134,7 +137,15 @@ public class BasePolarisCatalog extends BaseMetastoreViewCatalog
"INITIALIZE_DEFAULT_CATALOG_FILEIO_FOR_TEST";
static final boolean INITIALIZE_DEFAULT_CATALOG_FILEIO_FOR_TEST_DEFAULT = false;
- private static final int MAX_RETRIES = 12;
+ @VisibleForTesting
+ public static final Set<Integer> RETRYABLE_AZURE_HTTP_CODES =
Review Comment:
Updated!
--
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] More sensible BasePolarisCatalog retry behavior [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b merged PR #1046:
URL: https://github.com/apache/polaris/pull/1046
--
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] More sensible BasePolarisCatalog retry behavior [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #1046:
URL: https://github.com/apache/polaris/pull/1046#discussion_r1970822261
##########
service/common/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java:
##########
@@ -1484,7 +1501,12 @@ public void doRefresh() {
refreshFromMetadataLocation(
latestLocation,
SHOULD_RETRY_REFRESH_PREDICATE,
- MAX_RETRIES,
+ callContext
Review Comment:
Can we try to remove the duplicated code in line 1274 and 1505?
##########
service/common/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java:
##########
@@ -134,7 +137,15 @@ public class BasePolarisCatalog extends BaseMetastoreViewCatalog
"INITIALIZE_DEFAULT_CATALOG_FILEIO_FOR_TEST";
static final boolean INITIALIZE_DEFAULT_CATALOG_FILEIO_FOR_TEST_DEFAULT = false;
- private static final int MAX_RETRIES = 12;
+ @VisibleForTesting
+ public static final Set<Integer> RETRYABLE_AZURE_HTTP_CODES =
Review Comment:
Can we move this field along with methods `isStorageProviderRetryableException` and `isAccessDenied` to a dedicated class? The class `IcebergExceptionMapper` could be a candidate.
##########
polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java:
##########
@@ -264,4 +264,13 @@ public static <T> Builder<T> builder() {
+ STORAGE_CREDENTIAL_DURATION_SECONDS.key)
.defaultValue(30 * 60) // 30 minutes
.build();
+
+ // The default value is kept at 12 for backward compatibility
+ public static final PolarisConfiguration<Integer> MAX_METADATA_REFRESH_RETRIES =
+ PolarisConfiguration.<Integer>builder()
+ .key("MAX_METADATA_REFRESH_RETRIES")
+ .description(
+ "How many times to retry refreshing metadata when the previous error was retryable")
+ .defaultValue(12)
+ .build();
Review Comment:
> These retries are on top of SDK retries
I believe so. This is done by Iceberg `Task` under the hood. Is it possible to make the default is reasonable number while changing the current deployment config? The concern is that users normally don't change the default value until they hit issues.
##########
service/common/src/main/java/org/apache/polaris/service/exception/IcebergExceptionMapper.java:
##########
@@ -158,18 +161,25 @@ static int mapExceptionToResponseCode(RuntimeException rex) {
};
}
+ /**
+ * @return UNKNOWN_CLOUD_HTTP_CODE if the exception is not a cloud exception that we know how to
+ * extract the code from
+ */
Review Comment:
Can we have a correct format of Java doc if we're going to have one? for example
```
/**
* description
* @param rex
* @return UNKNOWN_CLOUD_HTTP_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] More sensible BasePolarisCatalog retry behavior [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #1046:
URL: https://github.com/apache/polaris/pull/1046#discussion_r1970861204
##########
polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java:
##########
@@ -264,4 +264,13 @@ public static <T> Builder<T> builder() {
+ STORAGE_CREDENTIAL_DURATION_SECONDS.key)
.defaultValue(30 * 60) // 30 minutes
.build();
+
+ // The default value is kept at 12 for backward compatibility
+ public static final PolarisConfiguration<Integer> MAX_METADATA_REFRESH_RETRIES =
+ PolarisConfiguration.<Integer>builder()
+ .key("MAX_METADATA_REFRESH_RETRIES")
+ .description(
+ "How many times to retry refreshing metadata when the previous error was retryable")
+ .defaultValue(12)
+ .build();
Review Comment:
> These retries are on top of SDK retries
I believe so. This is done by Iceberg `Task` under the hood. Is it possible to make the default a reasonable number while changing the current deployment config to 12 for compatibility? The concern is that users normally don't change the default value until they hit issues.
--
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] More sensible BasePolarisCatalog retry behavior [polaris]
Posted by "andrew4699 (via GitHub)" <gi...@apache.org>.
andrew4699 commented on code in PR #1046:
URL: https://github.com/apache/polaris/pull/1046#discussion_r1966372170
##########
service/common/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java:
##########
@@ -2014,6 +2025,12 @@ private Boolean getBooleanContextConfiguration(String configKey, boolean default
* @return true if the exception is retryable
*/
private static boolean isStorageProviderRetryableException(Exception ex) {
+ if (ex instanceof RuntimeException rex
+ && !RETRYABLE_CLOUD_EXCEPTION_CODES.contains(
+ IcebergExceptionMapper.extractHttpCodeFromCloudException(rex))) {
Review Comment:
That's a fair point. I changed it to use the retryable property for AWS/GCP. Azure doesn't have one though.
--
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