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