adutra commented on code in PR #12197:
URL: https://github.com/apache/iceberg/pull/12197#discussion_r1981429970
##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -138,148 +133,42 @@ boolean keepTokenRefreshed() {
OAuth2Properties.TOKEN_REFRESH_ENABLED_DEFAULT);
}
- @VisibleForTesting
- ScheduledExecutorService tokenRefreshExecutor() {
- if (!keepTokenRefreshed()) {
- return null;
- }
-
- if (null == tokenRefreshExecutor) {
- synchronized (S3V4RestSignerClient.class) {
- if (null == tokenRefreshExecutor) {
- tokenRefreshExecutor =
ThreadPools.newScheduledPool("s3-signer-token-refresh", 1);
- }
- }
- }
-
- return tokenRefreshExecutor;
- }
-
- private Cache<String, AuthSession> authSessionCache() {
- if (null == authSessionCache) {
- synchronized (S3V4RestSignerClient.class) {
- if (null == authSessionCache) {
- long expirationIntervalMs =
- PropertyUtil.propertyAsLong(
- properties(),
- CatalogProperties.AUTH_SESSION_TIMEOUT_MS,
- CatalogProperties.AUTH_SESSION_TIMEOUT_MS_DEFAULT);
-
- authSessionCache =
- Caffeine.newBuilder()
- .expireAfterAccess(Duration.ofMillis(expirationIntervalMs))
- .removalListener(
- (RemovalListener<String, AuthSession>)
- (id, auth, cause) -> {
- if (null != auth) {
- LOG.trace("Stopping refresh for AuthSession");
- auth.stopRefreshing();
- }
- })
- .build();
- }
- }
- }
-
- return authSessionCache;
- }
-
private RESTClient httpClient() {
if (null == httpClient) {
synchronized (S3V4RestSignerClient.class) {
if (null == httpClient) {
- httpClient =
+ authManager = AuthManagers.loadAuthManager("s3-signer",
properties());
+ HTTPClient client =
HTTPClient.builder(properties())
.uri(baseSignerUri())
.withObjectMapper(S3ObjectMapper.mapper())
.build();
+ ImmutableMap.Builder<String, String> properties =
+ ImmutableMap.<String, String>builder()
+ .putAll(properties())
+ .putAll(optionalOAuthParams())
+ .put(OAuth2Properties.OAUTH2_SERVER_URI, oauth2ServerUri())
+ .put(OAuth2Properties.TOKEN_REFRESH_ENABLED,
String.valueOf(keepTokenRefreshed()))
+ .put(OAuth2Properties.SCOPE, SCOPE);
+ String token = token().get();
+ if (null != token) {
+ properties.put(OAuth2Properties.TOKEN, token);
+ } else if (credentialProvided()) {
+ properties.put(OAuth2Properties.CREDENTIAL, credential());
+ }
+ authSession = authManager.catalogSession(client,
properties.buildKeepingLast());
Review Comment:
I pushed yet another change that I believe achieves 100% compatibility with
the old code. The change consists of preferring `token` over `credential` in
`OAuth2Manager.catalogSession()`. This way, if the properties contain a token,
that token is used, and no token fetch happens (I manually validated that.)
I still think it's best to use `OAuth2Manager.catalogSession()` here rather
than `tableSession()` – even if sometimes the FileIO is table-scoped. The
method is not well named imho; I offered a while ago to rename it to
`mainSession` or `genericSession`. If you agree it's still time to rename it.
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
