flyrain commented on code in PR #408:
URL: https://github.com/apache/polaris/pull/408#discussion_r1893309242
##########
polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java:
##########
@@ -89,6 +94,25 @@ public PolarisConfiguration<T> build() {
}
}
+ /**
+ * Returns the value of a `PolarisConfiguration`, or the default if it
cannot be loaded. This
+ * method does not need to be used when a `CallContext` is already available
+ */
+ public static <T> T loadConfig(PolarisConfiguration<T> configuration) {
Review Comment:
+1 on this method. There are a lot of other places can reuse this as well,
like here
https://github.com/polaris-catalog/polaris/blob/main/polaris-service/src/main/java/org/apache/polaris/service/catalog/BasePolarisCatalog.java#L1029-L1029.
We can consolidate them in a followup PR.
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsCredentialsStorageIntegration.java:
##########
@@ -69,13 +71,21 @@ public EnumMap<PolarisCredentialProperty, String>
getSubscopedCreds(
allowedReadLocations,
allowedWriteLocations)
.toJson())
+ .durationSeconds(
+ PolarisConfiguration.loadConfig(
+
PolarisConfiguration.STORAGE_CREDENTIAL_DURATION_SECONDS))
Review Comment:
Minor suggestion: we could keep them in one line like the following if we
import the static field/method.
```
.durationSeconds(loadConfig(STORAGE_CREDENTIAL_DURATION_SECONDS))
```
##########
polaris-core/src/main/java/org/apache/polaris/core/PolarisConfiguration.java:
##########
@@ -199,4 +223,22 @@ public static <T> Builder<T> builder() {
"If set to true, allows tables to be dropped with the purge
parameter set to true.")
.defaultValue(true)
.build();
+
+ public static final PolarisConfiguration<Integer>
STORAGE_CREDENTIAL_DURATION_SECONDS =
+ PolarisConfiguration.<Integer>builder()
+ .key("STORAGE_CREDENTIAL_DURATION_SECONDS")
+ .description(
+ "The duration of time that vended storage credentials are valid
for. Support for"
+ + " longer (or shorter) durations is dependent on the
storage provider.")
Review Comment:
Minor suggestion: adding a comment that GCS isn't supported yet?
--
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]
