[GitHub] [iceberg] rdblue commented on a diff in pull request #7046: AWS: Prevent creating refresh executor when token refresh is disabled

["rdblue (via GitHub)" <gi...@apache.org>]

rdblue commented on code in PR #7046:
URL: https://github.com/apache/iceberg/pull/7046#discussion_r1132652321


##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -70,10 +69,13 @@
   private static final Cache<Key, SignedComponent> SIGNED_COMPONENT_CACHE =
       Caffeine.newBuilder().expireAfterWrite(30, TimeUnit.SECONDS).maximumSize(100).build();
 
-  private static final ScheduledExecutorService TOKEN_REFRESH_EXECUTOR =
-      ThreadPools.newScheduledPool("s3-signer-token-refresh", 1);
   private static final String SCOPE = "sign";
-  private static final AtomicReference<RESTClient> HTTP_CLIENT_REF = new AtomicReference<>();
+
+  @SuppressWarnings("immutables:incompat")
+  private static volatile ScheduledExecutorService tokenRefreshExecutor;
+
+  @SuppressWarnings("immutables:incompat")
+  private static volatile RESTClient httpClient;

Review Comment:
   Since these are private static, they should use all caps for naming right?



##########
aws/src/main/java/org/apache/iceberg/aws/s3/signer/S3V4RestSignerClient.java:
##########
@@ -113,26 +115,44 @@ boolean keepTokenRefreshed() {
         OAuth2Properties.TOKEN_REFRESH_ENABLED_DEFAULT);
   }
 
+  private ScheduledExecutorService tokenRefreshExecutor() {

Review Comment:
   Shouldn't this be static?



-- 
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