[PLUGIN-1808] Retry policy to service account for 5xx errors for bigquery plugin by AnkitCLI · Pull Request #1544 · data-integrations/google-cloud

AnkitCLI · 2025-05-06T07:02:25Z

PLUGIN-1808 Added a retry policy to handle 5xx server errors when fetching the service account access token in the BigQuery plugin

itsankit-google

on a second thought, I feel just move all the retry configs & retry policy to ServiceAccountAccessTokenProvider class.

For start, we can use default configs and no need to make them configurable.

This will make the PR look cleaner and changes will be limited to one class.

itsankit-google · 2025-06-02T08:07:52Z


    private BigQuerySourceConfig(@Nullable BigQueryConnectorConfig connection, @Nullable String dataset,
-                                @Nullable String cmekKey, @Nullable String bucket, @Nullable String table) {
+                               @Nullable String cmekKey, @Nullable String bucket, @Nullable String table) {


nit: indentation fix

itsankit-google · 2025-06-03T07:58:56Z

+            }
+            return new AccessToken(token.getTokenValue(), token.getExpirationTime().getTime());
+          });
+      } catch (Exception e) {


extract cause from FailesafeException

itsankit-google · 2025-06-03T07:59:09Z

+          new ErrorCategory(ErrorCategoryEnum.PLUGIN),
+          "Unable to get service account access token after retries.",
+          e.getMessage(),
+          ErrorType.UNKNOWN,


for server error use SYSTEM error type

itsankit-google · 2025-06-03T08:00:29Z

  private Configuration conf;
  private GoogleCredentials credentials;
  private static final Gson GSON = new Gson();
+  private static final Logger logger = LoggerFactory.getLogger(ServiceAccountAccessTokenProvider.class);


usually use private static final variables in uppercase: logger -> LOG

itsankit-google · 2025-06-04T08:38:23Z

+    int initialRetryDuration = DEFAULT_INITIAL_RETRY_DURATION_SECONDS;
+    int maxRetryCount = DEFAULT_MAX_RETRY_COUNT;
+    int maxRetryDuration =  DEFAULT_MAX_RETRY_DURATION_SECONDS;


why are we creating copy of instance variables here? It eliminates the purpose of them being final & static

itsankit-google · 2025-06-04T08:39:21Z

+  private RetryPolicy<Object> getRetryPolicy(int initialRetryDuration, int maxRetryDuration,
+                                             int maxRetryCount) {
+    return RetryPolicy.builder()
+      .handle(ServerErrorException.class)
+      .withBackoff(Duration.ofSeconds(initialRetryDuration), Duration.ofSeconds(maxRetryDuration))
+      .withMaxRetries(maxRetryCount)
+      .onRetry(event -> LOG.debug("Retry attempt {} due to {}", event.getAttemptCount(), event.getLastException().
+        getMessage()))
+      .onSuccess(event -> LOG.debug("Access Token Fetched Successfully."))
+      .onRetriesExceeded(event -> LOG.error("Retry limit reached for Service account."))
+      .build();
+  }


this can be a private static method & retry policy can have a static final instance variable rather than creating retry policy for every call.

itsankit-google · 2025-06-05T06:06:39Z

+          new ErrorCategory(ErrorCategoryEnum.PLUGIN),
+          "Unable to get service account access token after retries.",
+          t.getMessage(),
+          ErrorType.SYSTEM,


getCause() should match first check whether it was a ServerException then only use SYSTEM otherwise UNKNOWN.

itsankit-google · 2025-06-05T06:14:42Z

      throw ErrorUtils.getProgramFailureException(new ErrorCategory(ErrorCategoryEnum.PLUGIN),
        "Unable to refresh service account access token.", e.getMessage(),
        ErrorType.UNKNOWN, true, e);


better to use

google-cloud/src/main/java/io/cdap/plugin/gcp/common/GCPErrorDetailsProviderUtil.java

Line 117 in c13bece

public static ProgramFailureException getHttpResponseExceptionDetailsFromChain(Throwable e, String errorReason,

with externalDocUrl as https://cloud.google.com/compute/docs/troubleshooting/troubleshoot-metadata-server

itsankit-google · 2025-06-05T06:14:51Z

+          });
+      } catch (FailsafeException e) {
+        Throwable t = e.getCause() != null ? e.getCause() : e;
+        throw ErrorUtils.getProgramFailureException(


better to use

google-cloud/src/main/java/io/cdap/plugin/gcp/common/GCPErrorDetailsProviderUtil.java

Line 117 in c13bece

public static ProgramFailureException getHttpResponseExceptionDetailsFromChain(Throwable e, String errorReason,

with externalDocUrl as https://cloud.google.com/compute/docs/troubleshooting/troubleshoot-metadata-server

itsankit-google · 2025-06-05T08:28:44Z

  public static final int BQ_DEFAULT_READ_TIMEOUT_SECONDS = 120;
  public static final String DATASTORE_SUPPORTED_DOC_URL = "https://cloud.google.com/datastore/docs/concepts/errors";
  public static final String BIG_TABLE_SUPPORTED_DOC_URL = "https://cloud.google.com/bigtable/docs/status-codes";
+  public static final String SERVER_ERROR_SUPPORTED_DOC_URL =


GCE_METADATA_SERVER_ERROR_SUPPORTED_DOC_URL

itsankit-google · 2025-06-05T08:30:34Z

+    return msg != null && msg.matches("^5\\d{2}$"); // crude check for 5xx codes
+  }
+
+  private com.google.auth.oauth2.AccessToken safeGetAccessToken() throws IOException {


what do we mean by safeGetAccessToken here?

Usually method names should indicate what are we trying to do here.

changed to retrieveAccessToken

itsankit-google · 2025-06-05T08:42:33Z

+      LOG.debug(
+        "Initializing RetryPolicy with the following configuration: MaxRetryCount: {}, InitialRetryDuration: {}s, " +
+          "MaxRetryDuration: {}s", DEFAULT_MAX_RETRY_COUNT, DEFAULT_INITIAL_RETRY_DURATION_SECONDS,
+        DEFAULT_MAX_RETRY_DURATION_SECONDS);


no need of this log for every access token fetch.

This can be present in the constructor.

itsankit-google · 2025-06-05T08:43:00Z

@@ -64,9 +118,13 @@ public void refresh() throws IOException {
    try {
      getCredentials().refresh();


We should also add retries on getCredentials().refresh()

itsankit-google · 2025-06-05T09:24:16Z

+
+  private boolean isServerError(IOException e) {
+    String msg = e.getMessage();
+    return msg != null && msg.matches("^5\\d{2}$"); // crude check for 5xx codes


We should broaden the matching regex to match something like :

Unexpected Error code 5xx trying to get security access token from Compute Engine metadata for the default service account

And we could also keep the compiled pattern as prival final static variable instead of compiling it for every call.

Regex matches are cheap but pattern compilation is costly.

itsankit-google · 2025-06-10T08:58:02Z

+  public static final int DEFAULT_MAX_RETRY_COUNT = 5;
+  public static final int DEFAULT_MAX_RETRY_DURATION_SECONDS = 80;
+  private static final RetryPolicy<Object> RETRY_POLICY = createRetryPolicy();
+  private static final Pattern SERVER_ERROR_PATTERN = Pattern.compile(".*5\\d{2}.*");


This comment is still not addressed : #1544 (comment)

We are still looking at very broad pattern of just finding 5xx in the error message.

The pattern is broad as we are not sure of the error message and are only matching on 5xx.
Is there a way get the error message structure to narrow this regex match.

see this class : https://github.com/googleapis/google-auth-library-java/blob/d02ab85ae8d29402650d3cbf00e00363a903fe31/oauth2_http/java/com/google/auth/oauth2/ComputeEngineCredentials.java#L353-L354

We already know from where the error message is coming. Please look at the stacktrace of errors in the JIRA.

itsankit-google

Can you please add a unit test?

itsankit-google

please also add unit tests to check retries, for example : https://github.com/cdapio/cdap/pull/15226/files#diff-acc8b9b649f280ee45108ce0220bc742b327acd041f1c1022489452f021ad852R53-R69

AnkitCLI changed the title ~~Retry policy for service account~~ Retry policy to service account for 5xx errros May 6, 2025

AnkitCLI changed the title ~~Retry policy to service account for 5xx errros~~ Retry policy to service account for 5xx errors May 6, 2025

AnkitCLI marked this pull request as ready for review May 6, 2025 07:05

AnkitCLI added the build Trigger unit test build label May 6, 2025

AnkitCLI changed the title ~~Retry policy to service account for 5xx errors~~ Retry policy to service account for 5xx errors for bigquery plugin May 6, 2025

itsankit-google changed the title ~~Retry policy to service account for 5xx errors for bigquery plugin~~ [PLUGIN-1808] Retry policy to service account for 5xx errors for bigquery plugin Jun 2, 2025

vsethi09 requested review from itsankit-google, ritwiksahani and sahusanket June 2, 2025 08:11

itsankit-google reviewed Jun 2, 2025

View reviewed changes

AnkitCLI removed the build Trigger unit test build label Jun 3, 2025

AnkitCLI force-pushed the AddingRetryWithFailsafe branch 3 times, most recently from 7eb0bb3 to 46c5d1e Compare June 3, 2025 07:08

AnkitCLI added the build Trigger unit test build label Jun 3, 2025

itsankit-google reviewed Jun 3, 2025

View reviewed changes

Comment thread src/main/java/io/cdap/plugin/gcp/gcs/ServiceAccountAccessTokenProvider.java

itsankit-google reviewed Jun 4, 2025

View reviewed changes

Comment thread src/main/java/io/cdap/plugin/gcp/common/ServerErrorException.java

AnkitCLI force-pushed the AddingRetryWithFailsafe branch 2 times, most recently from d52b351 to 1011aff Compare June 5, 2025 06:03

itsankit-google reviewed Jun 5, 2025

View reviewed changes

AnkitCLI force-pushed the AddingRetryWithFailsafe branch from 1011aff to 4bf53f8 Compare June 5, 2025 07:25

itsankit-google reviewed Jun 5, 2025

View reviewed changes

AnkitCLI requested a review from itsankit-google June 6, 2025 09:44

itsankit-google requested changes Jun 10, 2025

View reviewed changes

AnkitCLI force-pushed the AddingRetryWithFailsafe branch 2 times, most recently from 39dab19 to 2cd1c91 Compare June 16, 2025 06:20

AnkitCLI added build Trigger unit test build and removed build Trigger unit test build labels Jun 16, 2025

AnkitCLI requested a review from itsankit-google June 23, 2025 04:54

AnkitCLI requested review from itsankit-google and removed request for itsankit-google July 1, 2025 05:34

itsankit-google reviewed Jul 1, 2025

View reviewed changes

itsankit-google reviewed Jul 4, 2025

View reviewed changes

Comment thread src/main/java/io/cdap/plugin/gcp/gcs/ServiceAccountAccessTokenProvider.java

itsankit-google reviewed Jul 4, 2025

View reviewed changes

itsankit-google approved these changes Jul 7, 2025

View reviewed changes

Retry policy for service account

acd0f0d

AnkitCLI force-pushed the AddingRetryWithFailsafe branch from dea8c9c to acd0f0d Compare July 8, 2025 05:44

AnkitCLI merged commit e4aa851 into data-integrations:develop Jul 8, 2025
16 checks passed

AnkitCLI mentioned this pull request Jul 8, 2025

[🍒] [PLUGIN-1808]Retry policy for service account for 5xx errors #1567

Merged

		@@ -64,9 +118,13 @@ public void refresh() throws IOException {
		try {
		getCredentials().refresh();

Uh oh!

Conversation

AnkitCLI commented May 6, 2025 • edited by atlassian Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itsankit-google left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itsankit-google Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itsankit-google Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itsankit-google Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

itsankit-google Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnkitCLI commented May 6, 2025 •

edited by atlassian Bot

Loading

itsankit-google Jun 3, 2025 •

edited

Loading

itsankit-google Jun 3, 2025 •

edited

Loading

itsankit-google Jun 3, 2025 •

edited

Loading

itsankit-google Jun 5, 2025 •

edited

Loading