[GitHub] [pulsar] eolivelli commented on a change in pull request #13951: [OAuth2] Enable configurable preemptive token refresh in Java Client

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

eolivelli commented on a change in pull request #13951:
URL: https://github.com/apache/pulsar/pull/13951#discussion_r792333682



##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/auth/oauth2/AuthenticationOAuth2.java
##########
@@ -96,21 +127,71 @@ public void start() throws PulsarClientException {
         flow.initialize();
     }
 
+    /**
+     * The first time that this method is called, it retrieves a token. All subsequent
+     * calls should get a cached value. However, if there is an issue with the Identity
+     * Provider, there is a chance that the background thread responsible for keeping
+     * the refresh token hot will
+     * @return The authentication data identifying this client that will be sent to the broker
+     * @throws PulsarClientException
+     */
     @Override
     public synchronized AuthenticationDataProvider getAuthData() throws PulsarClientException {
         if (this.cachedToken == null || this.cachedToken.isExpired()) {
-            TokenResult tr = this.flow.authenticate();
-            this.cachedToken = new CachedToken(tr);
+            this.authenticate();
         }
         return this.cachedToken.getAuthData();
     }
 
+    /**
+     * Retrieve the token (synchronously), and then schedule refresh runnable.
+     */
+    private void authenticate() throws PulsarClientException {
+        if (log.isDebugEnabled()) {
+            log.debug("Attempting to retrieve OAuth2 token now.");
+        }
+        TokenResult tr = this.flow.authenticate();
+        this.cachedToken = new CachedToken(tr);
+        handleSuccessfulTokenRefresh();
+    }
+
+    private void handleSuccessfulTokenRefresh() {
+        if (scheduler != null) {
+            backoff.reset();
+            long expiresInMillis = TimeUnit.SECONDS.toMillis(cachedToken.latest.getExpiresIn());
+            scheduleRefresh((long) (expiresInMillis * expiryAdjustment));
+        }
+    }
+
+    /**
+     * Attempt to refresh the token. If successful, schedule the next refresh task according to the
+     * {@link #expiryAdjustment}. If failed, schedule another attempt to refresh the token according to the
+     * {@link #backoff} policy.
+     */
+    private void refreshToken() {
+        try {
+            this.authenticate();
+        } catch (Throwable e) {

Review comment:
       Catching Throwable is always a code smell and we should not do it if there is no strong reason.
   What about catching specific exceptions?
   If you are afraid of RuntimeExceptions that may break the scheduled task, @lhotari recently contributed support for a utility to address this problem (I can't recall the name currently 
   )




-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org