You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nastra (via GitHub)" <gi...@apache.org> on 2026/03/20 08:44:45 UTC
[PR] GCP: Add scheduled refresh for storage credentials held by GCSFileIO [iceberg]
nastra opened a new pull request, #15696:
URL: https://github.com/apache/iceberg/pull/15696
The `GCSFileIO` implementation never refreshes the credentials that are held directly from the table load. The GCS storage clients use a `OAuth2RefreshCredentialsHandler` that internally refreshes the storage client, but those updates are not reflected back to the FileIO.
If an `GCSFileIO` instance is serialized to remote workers, the credentials may be expired triggering a thundering herd of requests to refresh immediately.
This change addresses this problem by proactively updating the credentials in the FileIO so that only valid credentials are propagated to remote clients.
This applies the same changes as was already done for S3FileIO in https://github.com/apache/iceberg/pull/15678
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] GCP: Add scheduled refresh for storage credentials held by GCSFileIO [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #15696:
URL: https://github.com/apache/iceberg/pull/15696#discussion_r2967164251
##########
gcp/src/main/java/org/apache/iceberg/gcp/gcs/GCSFileIO.java:
##########
@@ -199,13 +211,67 @@ private Map<String, PrefixedStorage> storageByPrefix() {
storageSupplier));
});
this.storageByPrefix = localStorageByPrefix;
+ scheduleCredentialRefresh();
}
}
}
return storageByPrefix;
}
+ private void scheduleCredentialRefresh() {
+ storageCredentials.stream()
+ .map(
+ storageCredential ->
+ storageCredential.config().get(GCPProperties.GCS_OAUTH2_TOKEN_EXPIRES_AT))
+ .filter(Objects::nonNull)
+ .map(expiresAtString -> Instant.ofEpochMilli(Long.parseLong(expiresAtString)))
+ .min(Comparator.naturalOrder())
+ .ifPresent(
+ expiresAt -> {
+ Instant prefetchAt = expiresAt.minus(5, ChronoUnit.MINUTES);
+ long delay = Duration.between(Instant.now(), prefetchAt).toMillis();
+ this.refreshFuture =
+ executorService()
+ .schedule(this::refreshStorageCredentials, delay, TimeUnit.MILLISECONDS);
+ });
+ }
+
+ private void refreshStorageCredentials() {
+ if (isResourceClosed.get()) {
+ return;
+ }
+
+ try (OAuth2RefreshCredentialsHandler handler =
+ OAuth2RefreshCredentialsHandler.create(properties)) {
Review Comment:
> Typically, we're talking about possibly one invocation per hour (most queries will likely have none because the run within the timeout)
If we create the handler lazily, we can achieve the same.
> holding all of the http client thread pools and other resources is the more wasteful approach
should we trim down the http client pool to be way less than ~100 in the first place ? we technically don't need the whole HTTP Client pool (~100) for OAuth2RefreshCredsHandler ? its gonna be mostly call to rest server by picking a connection from the pool .
My main suggestion was we are creating burst of connections / threads which we are discarding anyways after the credentials call completes, hence was an expensive thought.
##########
gcp/src/main/java/org/apache/iceberg/gcp/gcs/GCSFileIO.java:
##########
@@ -199,13 +211,67 @@ private Map<String, PrefixedStorage> storageByPrefix() {
storageSupplier));
});
this.storageByPrefix = localStorageByPrefix;
+ scheduleCredentialRefresh();
}
}
}
return storageByPrefix;
}
+ private void scheduleCredentialRefresh() {
+ storageCredentials.stream()
+ .map(
+ storageCredential ->
+ storageCredential.config().get(GCPProperties.GCS_OAUTH2_TOKEN_EXPIRES_AT))
+ .filter(Objects::nonNull)
+ .map(expiresAtString -> Instant.ofEpochMilli(Long.parseLong(expiresAtString)))
+ .min(Comparator.naturalOrder())
+ .ifPresent(
+ expiresAt -> {
+ Instant prefetchAt = expiresAt.minus(5, ChronoUnit.MINUTES);
+ long delay = Duration.between(Instant.now(), prefetchAt).toMillis();
+ this.refreshFuture =
+ executorService()
+ .schedule(this::refreshStorageCredentials, delay, TimeUnit.MILLISECONDS);
+ });
+ }
+
+ private void refreshStorageCredentials() {
+ if (isResourceClosed.get()) {
+ return;
+ }
+
+ try (OAuth2RefreshCredentialsHandler handler =
+ OAuth2RefreshCredentialsHandler.create(properties)) {
Review Comment:
> Typically, we're talking about possibly one invocation per hour (most queries will likely have none because the run within the timeout)
If we create the handler lazily, we can achieve the same.
> holding all of the http client thread pools and other resources is the more wasteful approach
should we trim down the http client pool to be way less than ~100 in the first place ? we technically don't need the whole HTTP Client pool (~100) for OAuth2RefreshCredsHandler ? its gonna be mostly call to rest server by picking a connection from the pool .
My main suggestion was we are creating burst of connections / threads which we are discarding anyways after the credentials call completes, hence was an expensive
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] GCP: Add scheduled refresh for storage credentials held by GCSFileIO [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks merged PR #15696:
URL: https://github.com/apache/iceberg/pull/15696
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] GCP: Add scheduled refresh for storage credentials held by GCSFileIO [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15696:
URL: https://github.com/apache/iceberg/pull/15696#discussion_r2966591508
##########
gcp/src/main/java/org/apache/iceberg/gcp/gcs/GCSFileIO.java:
##########
@@ -199,13 +211,67 @@ private Map<String, PrefixedStorage> storageByPrefix() {
storageSupplier));
});
this.storageByPrefix = localStorageByPrefix;
+ scheduleCredentialRefresh();
}
}
}
return storageByPrefix;
}
+ private void scheduleCredentialRefresh() {
+ storageCredentials.stream()
+ .map(
+ storageCredential ->
+ storageCredential.config().get(GCPProperties.GCS_OAUTH2_TOKEN_EXPIRES_AT))
+ .filter(Objects::nonNull)
+ .map(expiresAtString -> Instant.ofEpochMilli(Long.parseLong(expiresAtString)))
+ .min(Comparator.naturalOrder())
+ .ifPresent(
+ expiresAt -> {
+ Instant prefetchAt = expiresAt.minus(5, ChronoUnit.MINUTES);
+ long delay = Duration.between(Instant.now(), prefetchAt).toMillis();
+ this.refreshFuture =
+ executorService()
+ .schedule(this::refreshStorageCredentials, delay, TimeUnit.MILLISECONDS);
+ });
+ }
+
+ private void refreshStorageCredentials() {
+ if (isResourceClosed.get()) {
+ return;
+ }
+
+ try (OAuth2RefreshCredentialsHandler handler =
+ OAuth2RefreshCredentialsHandler.create(properties)) {
Review Comment:
I don't feel like this is too expensive. Typically, we're talking about possibly one invocation per hour (most queries will likely have none because the run within the timeout), so holding all of the http client thread pools and other resources is the more wasteful approach.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] GCP: Add scheduled refresh for storage credentials held by GCSFileIO [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #15696:
URL: https://github.com/apache/iceberg/pull/15696#discussion_r2973394694
##########
gcp/src/main/java/org/apache/iceberg/gcp/gcs/GCSFileIO.java:
##########
@@ -199,13 +211,67 @@ private Map<String, PrefixedStorage> storageByPrefix() {
storageSupplier));
});
this.storageByPrefix = localStorageByPrefix;
+ scheduleCredentialRefresh();
}
}
}
return storageByPrefix;
}
+ private void scheduleCredentialRefresh() {
+ storageCredentials.stream()
+ .map(
+ storageCredential ->
+ storageCredential.config().get(GCPProperties.GCS_OAUTH2_TOKEN_EXPIRES_AT))
+ .filter(Objects::nonNull)
+ .map(expiresAtString -> Instant.ofEpochMilli(Long.parseLong(expiresAtString)))
+ .min(Comparator.naturalOrder())
+ .ifPresent(
+ expiresAt -> {
+ Instant prefetchAt = expiresAt.minus(5, ChronoUnit.MINUTES);
+ long delay = Duration.between(Instant.now(), prefetchAt).toMillis();
+ this.refreshFuture =
+ executorService()
+ .schedule(this::refreshStorageCredentials, delay, TimeUnit.MILLISECONDS);
+ });
+ }
+
+ private void refreshStorageCredentials() {
+ if (isResourceClosed.get()) {
+ return;
+ }
+
+ try (OAuth2RefreshCredentialsHandler handler =
+ OAuth2RefreshCredentialsHandler.create(properties)) {
+ List<StorageCredential> refreshed =
+ handler.fetchCredentials().credentials().stream()
+ .filter(c -> c.prefix().startsWith(ROOT_STORAGE_PREFIX))
+ .map(c -> StorageCredential.create(c.prefix(), c.config()))
+ .toList();
+
+ if (!refreshed.isEmpty() && !isResourceClosed.get()) {
+ this.storageCredentials = Lists.newArrayList(refreshed);
Review Comment:
added, but I think we have the same issue with S3FileIO. I've opened https://github.com/apache/iceberg/pull/15732
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] GCP: Add scheduled refresh for storage credentials held by GCSFileIO [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15696:
URL: https://github.com/apache/iceberg/pull/15696#discussion_r2967216991
##########
gcp/src/main/java/org/apache/iceberg/gcp/gcs/GCSFileIO.java:
##########
@@ -199,13 +211,67 @@ private Map<String, PrefixedStorage> storageByPrefix() {
storageSupplier));
});
this.storageByPrefix = localStorageByPrefix;
+ scheduleCredentialRefresh();
}
}
}
return storageByPrefix;
}
+ private void scheduleCredentialRefresh() {
+ storageCredentials.stream()
+ .map(
+ storageCredential ->
+ storageCredential.config().get(GCPProperties.GCS_OAUTH2_TOKEN_EXPIRES_AT))
+ .filter(Objects::nonNull)
+ .map(expiresAtString -> Instant.ofEpochMilli(Long.parseLong(expiresAtString)))
+ .min(Comparator.naturalOrder())
+ .ifPresent(
+ expiresAt -> {
+ Instant prefetchAt = expiresAt.minus(5, ChronoUnit.MINUTES);
+ long delay = Duration.between(Instant.now(), prefetchAt).toMillis();
+ this.refreshFuture =
+ executorService()
+ .schedule(this::refreshStorageCredentials, delay, TimeUnit.MILLISECONDS);
+ });
+ }
+
+ private void refreshStorageCredentials() {
+ if (isResourceClosed.get()) {
+ return;
+ }
+
+ try (OAuth2RefreshCredentialsHandler handler =
+ OAuth2RefreshCredentialsHandler.create(properties)) {
+ List<StorageCredential> refreshed =
+ handler.fetchCredentials().credentials().stream()
+ .filter(c -> c.prefix().startsWith(ROOT_STORAGE_PREFIX))
+ .map(c -> StorageCredential.create(c.prefix(), c.config()))
+ .toList();
+
+ if (!refreshed.isEmpty() && !isResourceClosed.get()) {
+ this.storageCredentials = Lists.newArrayList(refreshed);
Review Comment:
I think we need to schedule a new refresh at this point. Otherwise we never refresh after the first time.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] GCP: Add scheduled refresh for storage credentials held by GCSFileIO [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #15696:
URL: https://github.com/apache/iceberg/pull/15696#discussion_r2967164251
##########
gcp/src/main/java/org/apache/iceberg/gcp/gcs/GCSFileIO.java:
##########
@@ -199,13 +211,67 @@ private Map<String, PrefixedStorage> storageByPrefix() {
storageSupplier));
});
this.storageByPrefix = localStorageByPrefix;
+ scheduleCredentialRefresh();
}
}
}
return storageByPrefix;
}
+ private void scheduleCredentialRefresh() {
+ storageCredentials.stream()
+ .map(
+ storageCredential ->
+ storageCredential.config().get(GCPProperties.GCS_OAUTH2_TOKEN_EXPIRES_AT))
+ .filter(Objects::nonNull)
+ .map(expiresAtString -> Instant.ofEpochMilli(Long.parseLong(expiresAtString)))
+ .min(Comparator.naturalOrder())
+ .ifPresent(
+ expiresAt -> {
+ Instant prefetchAt = expiresAt.minus(5, ChronoUnit.MINUTES);
+ long delay = Duration.between(Instant.now(), prefetchAt).toMillis();
+ this.refreshFuture =
+ executorService()
+ .schedule(this::refreshStorageCredentials, delay, TimeUnit.MILLISECONDS);
+ });
+ }
+
+ private void refreshStorageCredentials() {
+ if (isResourceClosed.get()) {
+ return;
+ }
+
+ try (OAuth2RefreshCredentialsHandler handler =
+ OAuth2RefreshCredentialsHandler.create(properties)) {
Review Comment:
> Typically, we're talking about possibly one invocation per hour (most queries will likely have none because the run within the timeout)
If we create the handler lazily, we can achieve the same.
> holding all of the http client thread pools and other resources is the more wasteful approach
should we trim down the http client pool to be way less than ~100 in the first place ? we technically don't need the whole HTTP Client pool (~100) for OAuth2RefreshCredsHandler ? its gonna be mostly call to rest server by picking a connection from the pool .
My main suggestion was we are creating burst of connections / threads which we are discarding anyways after the credentials call completes, hence i though it was an expensive
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] GCP: Add scheduled refresh for storage credentials held by GCSFileIO [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15696:
URL: https://github.com/apache/iceberg/pull/15696#discussion_r2966579222
##########
gcp/src/main/java/org/apache/iceberg/gcp/gcs/GCSFileIO.java:
##########
@@ -199,13 +211,67 @@ private Map<String, PrefixedStorage> storageByPrefix() {
storageSupplier));
});
this.storageByPrefix = localStorageByPrefix;
+ scheduleCredentialRefresh();
Review Comment:
We should be refreshing at minimum 5 minutes before which is pretty consistent across implementations. There's always the chance that you could serialize right before that point, but it's actually pretty unlikely since most loads/scans start within seconds of access. It's probably better to think about jittering the refresh in the credential provider.
Just updating at serialization time is where I started originally, which is relatively easy for java serialization but not for kryo due to needing to register the class with kryo.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] GCP: Add scheduled refresh for storage credentials held by GCSFileIO [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #15696:
URL: https://github.com/apache/iceberg/pull/15696#discussion_r2966028155
##########
gcp/src/main/java/org/apache/iceberg/gcp/gcs/GCSFileIO.java:
##########
@@ -199,13 +211,67 @@ private Map<String, PrefixedStorage> storageByPrefix() {
storageSupplier));
});
this.storageByPrefix = localStorageByPrefix;
+ scheduleCredentialRefresh();
Review Comment:
[optional] i wonder if its just about making sure before the obj is serialized, we have set of credentials so that the executor when deserializes the obj, it does not hit the server with bcz the creds in FileIO obj expired a while ago ....
can we override the writeObj() (which JavaSerializer would call) which does the creds call and then just make this call instead of the periodic refresh ?
I know there are rough edges for Kryo i was reading there is `writeReplace()` which works for both
##########
gcp/src/main/java/org/apache/iceberg/gcp/gcs/GCSFileIO.java:
##########
@@ -199,13 +211,67 @@ private Map<String, PrefixedStorage> storageByPrefix() {
storageSupplier));
});
this.storageByPrefix = localStorageByPrefix;
+ scheduleCredentialRefresh();
}
}
}
return storageByPrefix;
}
+ private void scheduleCredentialRefresh() {
+ storageCredentials.stream()
+ .map(
+ storageCredential ->
+ storageCredential.config().get(GCPProperties.GCS_OAUTH2_TOKEN_EXPIRES_AT))
+ .filter(Objects::nonNull)
+ .map(expiresAtString -> Instant.ofEpochMilli(Long.parseLong(expiresAtString)))
+ .min(Comparator.naturalOrder())
+ .ifPresent(
+ expiresAt -> {
+ Instant prefetchAt = expiresAt.minus(5, ChronoUnit.MINUTES);
+ long delay = Duration.between(Instant.now(), prefetchAt).toMillis();
+ this.refreshFuture =
+ executorService()
+ .schedule(this::refreshStorageCredentials, delay, TimeUnit.MILLISECONDS);
+ });
+ }
+
+ private void refreshStorageCredentials() {
+ if (isResourceClosed.get()) {
+ return;
+ }
+
+ try (OAuth2RefreshCredentialsHandler handler =
+ OAuth2RefreshCredentialsHandler.create(properties)) {
Review Comment:
creating a OAuth2RefreshCredentialsHandler per refresh call would be expensive, it creates a http client and an AuthManager additionally, i wonder if we can just have this done one (invalidated / created again if props change) and just reuse that across refreshes ?
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] GCP: Add scheduled refresh for storage credentials held by GCSFileIO [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #15696:
URL: https://github.com/apache/iceberg/pull/15696#discussion_r2967164251
##########
gcp/src/main/java/org/apache/iceberg/gcp/gcs/GCSFileIO.java:
##########
@@ -199,13 +211,67 @@ private Map<String, PrefixedStorage> storageByPrefix() {
storageSupplier));
});
this.storageByPrefix = localStorageByPrefix;
+ scheduleCredentialRefresh();
}
}
}
return storageByPrefix;
}
+ private void scheduleCredentialRefresh() {
+ storageCredentials.stream()
+ .map(
+ storageCredential ->
+ storageCredential.config().get(GCPProperties.GCS_OAUTH2_TOKEN_EXPIRES_AT))
+ .filter(Objects::nonNull)
+ .map(expiresAtString -> Instant.ofEpochMilli(Long.parseLong(expiresAtString)))
+ .min(Comparator.naturalOrder())
+ .ifPresent(
+ expiresAt -> {
+ Instant prefetchAt = expiresAt.minus(5, ChronoUnit.MINUTES);
+ long delay = Duration.between(Instant.now(), prefetchAt).toMillis();
+ this.refreshFuture =
+ executorService()
+ .schedule(this::refreshStorageCredentials, delay, TimeUnit.MILLISECONDS);
+ });
+ }
+
+ private void refreshStorageCredentials() {
+ if (isResourceClosed.get()) {
+ return;
+ }
+
+ try (OAuth2RefreshCredentialsHandler handler =
+ OAuth2RefreshCredentialsHandler.create(properties)) {
Review Comment:
> Typically, we're talking about possibly one invocation per hour (most queries will likely have none because the run within the timeout)
If we create the handler lazily, we can achieve the same.
> holding all of the http client thread pools and other resources is the more wasteful approach
should we trim down the http client pool to be way less than ~100 in the first place ? we technically don't need the whole HTTP Client pool (~100) for OAuth2RefreshCredsHandler (though i think pools connections might be lazily made) ? its gonna be mostly call to rest server by picking a connection from the pool .
My main suggestion was we are creating burst of connections / threads which we are discarding anyways after the credentials call completes, hence i though it was an expensive
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] GCP: Add scheduled refresh for storage credentials held by GCSFileIO [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #15696:
URL: https://github.com/apache/iceberg/pull/15696#discussion_r2967164251
##########
gcp/src/main/java/org/apache/iceberg/gcp/gcs/GCSFileIO.java:
##########
@@ -199,13 +211,67 @@ private Map<String, PrefixedStorage> storageByPrefix() {
storageSupplier));
});
this.storageByPrefix = localStorageByPrefix;
+ scheduleCredentialRefresh();
}
}
}
return storageByPrefix;
}
+ private void scheduleCredentialRefresh() {
+ storageCredentials.stream()
+ .map(
+ storageCredential ->
+ storageCredential.config().get(GCPProperties.GCS_OAUTH2_TOKEN_EXPIRES_AT))
+ .filter(Objects::nonNull)
+ .map(expiresAtString -> Instant.ofEpochMilli(Long.parseLong(expiresAtString)))
+ .min(Comparator.naturalOrder())
+ .ifPresent(
+ expiresAt -> {
+ Instant prefetchAt = expiresAt.minus(5, ChronoUnit.MINUTES);
+ long delay = Duration.between(Instant.now(), prefetchAt).toMillis();
+ this.refreshFuture =
+ executorService()
+ .schedule(this::refreshStorageCredentials, delay, TimeUnit.MILLISECONDS);
+ });
+ }
+
+ private void refreshStorageCredentials() {
+ if (isResourceClosed.get()) {
+ return;
+ }
+
+ try (OAuth2RefreshCredentialsHandler handler =
+ OAuth2RefreshCredentialsHandler.create(properties)) {
Review Comment:
> Typically, we're talking about possibly one invocation per hour (most queries will likely have none because the run within the timeout)
If we create the handler lazily, we can achieve the same.
> holding all of the http client thread pools and other resources is the more wasteful approach
should we trim down the http client pool to be 1 in the first place ? we technically don't need the whole HTTP Client pool (~100) for OAuth2RefreshCredsHandler ? its gonna be mostly single threaded.
My main suggestion was we are creating burst of connections / threads which we are discarding anyways after the credentials call completes, hence was an expensive thought.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] GCP: Add scheduled refresh for storage credentials held by GCSFileIO [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #15696:
URL: https://github.com/apache/iceberg/pull/15696#discussion_r2966185571
##########
gcp/src/main/java/org/apache/iceberg/gcp/gcs/GCSFileIO.java:
##########
@@ -199,13 +211,67 @@ private Map<String, PrefixedStorage> storageByPrefix() {
storageSupplier));
});
this.storageByPrefix = localStorageByPrefix;
+ scheduleCredentialRefresh();
Review Comment:
One more thing this can benefit is lets say default cred expiry is ~60 min and we serialized the io at 54th minute (assuming 5 min refresh interval) ,,,, the executors will always hit the creds endpoint within 5 min, but in this way it will not even have to for tasks with time 60 min
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org