[GitHub] [iceberg] bryanck commented on a diff in pull request #4991: Improved shutdown of token refresh executor during REST catalog close

[GitBox <gi...@apache.org>]

bryanck commented on code in PR #4991:
URL: https://github.com/apache/iceberg/pull/4991#discussion_r891692161


##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -361,15 +362,25 @@ private void scheduleTokenRefresh(
 
   @Override
   public void close() throws IOException {
+    shutdownRefreshExecutor();
+
     if (client != null) {
       client.close();
     }
+  }
 
+  private void shutdownRefreshExecutor() {
     if (refreshExecutor != null) {
       ScheduledExecutorService service = refreshExecutor;
       this.refreshExecutor = null;
 
-      service.shutdown();
+      List<Runnable> tasks = service.shutdownNow();
+      tasks.forEach(task -> {
+        if (task instanceof Future) {
+          ((Future<?>) task).cancel(true);
+        }
+      });

Review Comment:
   If we did that, we'd almost always await termination, as the next scheduled task will be pending, so we don't want to wait for it to run (it could be scheduled several minutes or more in the future). If a refresh task is actually running, letting it finish doesn't really add benefit as the response won't be used for anything once the catalog is closed.



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