[PECOBLR-1219] Added Api retriable codes URL param #1110

nishkarsh-db · 2025-11-27T05:30:00Z

Description

Added a new URL param apiRetriableCodes in order to retry thrift requests even when Retry-After header is not present

Testing

Added unit test for the same

Copilot

Pull request overview

This PR introduces a new URL parameter apiRetriableCodes that enables retrying HTTP requests for specified status codes even when the Retry-After header is absent. When such codes are encountered without a retry header, the system falls back to exponential backoff delay calculation.

Key Changes:

Added apiRetriableCodes URL parameter to configure custom retriable HTTP status codes
Modified retry handler logic to use exponential backoff for configured codes when Retry-After header is missing
Added test coverage for the new retry behavior

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
DatabricksJdbcUrlParams.java	Defines the new `API_RETRIABLE_CODES` URL parameter with description
IDatabricksConnectionContext.java	Adds interface method to retrieve configured API retriable codes
DatabricksConnectionContext.java	Implements parsing of comma-separated status codes with error handling
DatabricksHttpRetryHandler.java	Implements retry logic using exponential backoff for custom retriable codes
DatabricksHttpRetryHandlerTest.java	Adds test verifying retry behavior for 503 status without Retry-After header

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-27T05:37:13Z

src/main/java/com/databricks/jdbc/dbclient/impl/http/DatabricksHttpRetryHandler.java

+      retryInterval = (int) (calculateExponentialBackoff(executionCount) / 1000);
+    }
+


This exponential backoff is applied unconditionally for all cases where retryInterval == -1, including status codes not in apiRetriableCodes. The logic should only apply exponential backoff when isInCustomRetriableCodes is true. For other cases with retryInterval == -1, the method should return false as per the existing logic (lines 149-161).

Suggested change

retryInterval = (int) (calculateExponentialBackoff(executionCount) / 1000);

}

if (isInCustomRetriableCodes) {

retryInterval = (int) (calculateExponentialBackoff(executionCount) / 1000);

} else {

return false;

}

}

This condition is never reached. And if it reaches (in future) then this will not be a problem.

src/main/java/com/databricks/jdbc/api/impl/DatabricksConnectionContext.java

src/main/java/com/databricks/jdbc/dbclient/impl/http/DatabricksHttpRetryHandler.java

vikrantpuppala · 2025-11-27T09:24:22Z

src/main/java/com/databricks/jdbc/dbclient/impl/http/DatabricksHttpRetryHandler.java

+      apiRetriableCodes = connectionContext.getApiRetriableCodes();
+    }
+    boolean isInCustomRetriableCodes =
+        apiRetriableCodes != null && apiRetriableCodes.contains(statusCode);


but wouldn't request be already rejected at line 133 (!isStatusCodeRetryable)

In case of 503/429 with no retry-after header, it will not be rejected (unless set in JDBC URL params) as isStatusCodeRetryable just checks the JDBC URL params.

but the new param says apiRetriableCodes which means if i supply 501 to be retried then it should be. it is not specific to 503/429

Ok, so i think the parameter definition could be changed, but i don't think that bypassing the isStatusCodeRetryable check is something we want.

vikrantpuppala · 2025-11-27T10:45:38Z

src/main/java/com/databricks/jdbc/dbclient/impl/http/DatabricksHttpRetryHandler.java

+    // Get API retriable codes from connection context
+    Set<Integer> apiRetriableCodes = connectionContext.getApiRetriableCodes();
+    boolean isInCustomRetriableCodes =
+        apiRetriableCodes != null && apiRetriableCodes.contains(statusCode);


apiRetriableCodes can never be null?

Yes, It can never be null, So, removed that check.

gopalldb · 2025-11-28T10:49:44Z

src/main/java/com/databricks/jdbc/common/DatabricksJdbcUrlParams.java

+      "Comma-separated list of HTTP status codes that should be retried even without Retry-After header.",
+      ""),
+  API_CODES_RETRY_TIMEOUT(
+      "ApiCodesRetryTimeout", "Timeout for retrying API retriable codes", "120");


Shall we just rename this to ApiRetryTimeout? So that we can use this for some future generic use-cases

gopalldb · 2025-11-28T10:50:36Z

src/main/java/com/databricks/jdbc/common/DatabricksJdbcUrlParams.java

-      "EnableTokenFederation", "Enable token federation for authentication", "1");
+      "EnableTokenFederation", "Enable token federation for authentication", "1"),
+  API_RETRIABLE_CODES(
+      "ApiRetriableCodes",


ApiRetriableHttpCodes

gopalldb · 2025-11-28T10:52:44Z

src/main/java/com/databricks/jdbc/api/impl/DatabricksConnectionContext.java

+
+    Set<Integer> retriableCodes = new HashSet<>();
+    for (String code : codes.split(",")) {
+      try {


we should have generic parser, which can be used for parsing Set used elsewhere in this file as well, instead of duplicating code everywhere.

gopalldb · 2025-11-28T10:56:14Z

src/main/java/com/databricks/jdbc/dbclient/impl/http/DatabricksHttpRetryHandler.java


+    // check if retry timeout has been hit for custom API retriable codes
+    if (apiRetriableCodes.contains(statusCode)
+        && statusCode != HttpStatus.SC_SERVICE_UNAVAILABLE


why this filtering for 403 and 429 here? Why not also include 429 and 503

nishkarsh-db added 2 commits November 27, 2025 10:15

Added New URL Param to retry without retry-after header

b25fbf1

Rplaced hard-coded check in testApiRetriableCodesWithoutRetryAfterHeader

9db1a81

nishkarsh-db requested review from gopalldb and vikrantpuppala November 27, 2025 05:30

vikrantpuppala requested a review from Copilot November 27, 2025 05:36

Copilot AI reviewed Nov 27, 2025

View reviewed changes

added imports

b187146

nishkarsh-db changed the title ~~Added Api retriable codes URL param~~ [PECOBLR-1219] Added Api retriable codes URL param Nov 27, 2025

vikrantpuppala reviewed Nov 27, 2025

View reviewed changes

added test for ApiretriableCodes in connectioncontext

0c8c199

vikrantpuppala reviewed Nov 27, 2025

View reviewed changes

nishkarsh-db added 2 commits November 27, 2025 16:54

Removed unnecessary null check

9ba776b

Added new param for API_CODES_RETRY_TIMEOUT

bd74599

gopalldb reviewed Nov 28, 2025

View reviewed changes

		retryInterval = (int) (calculateExponentialBackoff(executionCount) / 1000);
		}

-      retryInterval = (int) (calculateExponentialBackoff(executionCount) / 1000);
-    }
+      if (isInCustomRetriableCodes) {
+        retryInterval = (int) (calculateExponentialBackoff(executionCount) / 1000);
+      } else {
+        return false;
+      }
+    }

[PECOBLR-1219] Added Api retriable codes URL param #1110

Are you sure you want to change the base?

[PECOBLR-1219] Added Api retriable codes URL param #1110

Uh oh!

Conversation

nishkarsh-db commented Nov 27, 2025

Description

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants