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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
