Skip to content

Commit 3e0490b

Browse files
renaudhartert-dbrenaudhartert-db
committed
Correctly parse 429 errors
1 parent ffbfe37 commit 3e0490b

File tree

3 files changed

+95
-64
lines changed

3 files changed

+95
-64
lines changed

‎databricks-sdk-java/src/main/java/com/databricks/sdk/core/ApiClient.java‎

Lines changed: 15 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -221,9 +221,6 @@ private Response executeInner(Request in, String path, RequestOptions options) {
221221
while (true) {
222222
attemptNumber++;
223223

224-
IOException err = null;
225-
Response out = null;
226-
227224
// Authenticate the request. Failures should not be retried.
228225
in.withHeaders(authenticateFunc.apply(null));
229226

@@ -239,27 +236,28 @@ private Response executeInner(Request in, String path, RequestOptions options) {
239236
userAgent += String.format(" auth/%s", authType);
240237
}
241238
in.withHeader("User-Agent", userAgent);
242-
243239
options.applyOptions(in);
244240

245-
// Make the request, catching any exceptions, as we may want to retry.
241+
Response response;
246242
try {
247-
out = httpClient.execute(in);
243+
response = httpClient.execute(in);
248244
if (LOG.isDebugEnabled()) {
249-
LOG.debug(makeLogRecord(in, out));
245+
LOG.debug(makeLogRecord(in, response));
250246
}
251247
} catch (IOException e) {
252-
err = e;
253248
LOG.debug("Request {} failed", in, e);
249+
// TODO: Evaluate whether this is actually the right thing to do
250+
// compared to propagating the exception "as is".
251+
throw new DatabricksError("IO_ERROR", 523, e);
254252
}
255253

256-
// Check if the request succeeded
257-
if (isRequestSuccessful(out, err)) {
258-
return out;
254+
if (isResponseSuccessful(response)) {
255+
return response; // stop here if the request succeeded
259256
}
260-
// The request did not succeed.
261-
// Check if the request cannot be retried: if yes, retry after backoff, else throw the error.
262-
DatabricksError databricksError = ApiErrors.getDatabricksError(out, err);
257+
258+
// The request did not succeed. Though, some errors are retriable and
259+
// should be retried with exponential backoff.
260+
DatabricksError databricksError = ApiErrors.getDatabricksError(response);
263261
if (!retryStrategy.isRetriable(databricksError)) {
264262
throw databricksError;
265263
}
@@ -269,7 +267,7 @@ private Response executeInner(Request in, String path, RequestOptions options) {
269267
}
270268

271269
// Retry after a backoff.
272-
long sleepMillis = getBackoffMillis(out, attemptNumber);
270+
long sleepMillis = getBackoffMillis(response, attemptNumber);
273271
LOG.debug(
274272
String.format("Retry %s in %dms", in.getRequestLine(), sleepMillis), databricksError);
275273
try {
@@ -281,9 +279,8 @@ private Response executeInner(Request in, String path, RequestOptions options) {
281279
}
282280
}
283281

284-
private boolean isRequestSuccessful(Response response, Exception e) {
285-
return e == null
286-
&& response.getStatusCode() >= 200
282+
private boolean isResponseSuccessful(Response response) {
283+
return response.getStatusCode() >= 200
287284
&& response.getStatusCode() < 300
288285
&& !PrivateLinkInfo.isPrivateLinkRedirect(response);
289286
}

‎databricks-sdk-java/src/main/java/com/databricks/sdk/core/error/ApiErrors.java‎

Lines changed: 77 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,11 @@
55
import com.databricks.sdk.core.error.details.ErrorDetails;
66
import com.databricks.sdk.core.http.Response;
77
import com.fasterxml.jackson.databind.ObjectMapper;
8-
import java.io.*;
8+
import com.google.api.client.util.Strings;
9+
import java.io.IOException;
10+
import java.io.InputStream;
911
import java.nio.charset.StandardCharsets;
12+
import java.util.Optional;
1013
import java.util.regex.Matcher;
1114
import java.util.regex.Pattern;
1215
import org.apache.commons.io.IOUtils;
@@ -16,73 +19,103 @@ public class ApiErrors {
1619
private static final ObjectMapper MAPPER = new ObjectMapper();
1720
private static final Pattern HTML_ERROR_REGEX = Pattern.compile("<pre>(.*)</pre>");
1821
private static final ErrorMapper ERROR_MAPPER = new ErrorMapper();
22+
private static final String SCIM_NULL_DETAILS = "null";
1923

20-
public static DatabricksError getDatabricksError(Response out, Exception error) {
21-
if (error != null) {
22-
// If the endpoint did not respond to the request, interpret the exception.
23-
return new DatabricksError("IO_ERROR", 523, error);
24-
} else if (out.getStatusCode() == 429) {
25-
return new DatabricksError("TOO_MANY_REQUESTS", "Current request has to be retried", 429);
24+
// Empty error details used to normalize errors with no error details.
25+
private static final ErrorDetails DEFAULT_ERROR_DETAILS = ErrorDetails.builder().build();
26+
27+
public static DatabricksError getDatabricksError(Response response) {
28+
// Private link error handling depends purely on the response URL without
29+
// looking at the error body.
30+
if (PrivateLinkInfo.isPrivateLinkRedirect(response)) {
31+
return ERROR_MAPPER.apply(response, new ApiErrorBody());
32+
}
33+
34+
Optional<ApiErrorBody> optionalErrorBody = parseApiError(response);
35+
if (!optionalErrorBody.isPresent()) {
36+
// Purely infer the error based on its status code.
37+
return ERROR_MAPPER.apply(response, new ApiErrorBody());
2638
}
2739

28-
ApiErrorBody errorBody = readErrorFromResponse(out);
29-
return ERROR_MAPPER.apply(out, errorBody);
40+
ApiErrorBody errorBody = optionalErrorBody.get();
41+
42+
// TODO: normalization should ideally happen at API call level,
43+
// allowing each method to control its own normalization.
44+
normalizeError(errorBody);
45+
46+
return ERROR_MAPPER.apply(response, errorBody);
3047
}
3148

32-
private static ApiErrorBody readErrorFromResponse(Response response) {
33-
// Private link error handling depends purely on the response URL.
34-
if (PrivateLinkInfo.isPrivateLinkRedirect(response)) {
35-
return new ApiErrorBody();
49+
private static void normalizeError(ApiErrorBody errorBody) {
50+
// Guarantee that all errors have an ErrorDetails container even
51+
// if it does not contains any specific error details.
52+
if (errorBody.getErrorDetails() == null) {
53+
errorBody.setErrorDetails(DEFAULT_ERROR_DETAILS);
3654
}
37-
ApiErrorBody errorBody = parseApiError(response);
3855

39-
// Condense API v1.2 and SCIM error string and code into the message and errorCode fields of
40-
// DatabricksApiException.
41-
if (errorBody.getApi12Error() != null && !errorBody.getApi12Error().isEmpty()) {
56+
// Error messages from older APIs used a different field to pass the
57+
// error message. We keep this code for backward compatibility until
58+
// these APIs are fully deprecated.
59+
if (!Strings.isNullOrEmpty(errorBody.getApi12Error())) {
4260
errorBody.setMessage(errorBody.getApi12Error());
61+
return;
62+
}
63+
64+
if (!Strings.isNullOrEmpty(errorBody.getMessage())) {
65+
return;
66+
}
67+
68+
// SCIM error handling.
69+
//
70+
// TODO: This code is brittle and should likely be refactored to a more
71+
// robust solution to detect SCIM errors. This will likely involve
72+
// parsing the SCIM error at the API call level rather than normalizing.
73+
if (Strings.isNullOrEmpty(errorBody.getScimDetail())) {
74+
return;
75+
}
76+
if (SCIM_NULL_DETAILS.equals(errorBody.getScimDetail())) {
77+
errorBody.setMessage("SCIM API Internal Error");
78+
} else {
79+
errorBody.setMessage(errorBody.getScimDetail());
4380
}
44-
if (errorBody.getMessage() == null || errorBody.getMessage().isEmpty()) {
45-
if (errorBody.getScimDetail() != null && !"null".equals(errorBody.getScimDetail())) {
46-
errorBody.setMessage(errorBody.getScimDetail());
47-
} else {
48-
errorBody.setMessage("SCIM API Internal Error");
49-
}
81+
if (!Strings.isNullOrEmpty(errorBody.getScimType())) {
5082
String message = errorBody.getScimType() + " " + errorBody.getMessage();
5183
errorBody.setMessage(message.trim());
52-
errorBody.setErrorCode("SCIM_" + errorBody.getScimStatus());
5384
}
54-
if (errorBody.getErrorDetails() == null) {
55-
errorBody.setErrorDetails(ErrorDetails.builder().build());
85+
if (!Strings.isNullOrEmpty(errorBody.getScimStatus())) {
86+
errorBody.setErrorCode("SCIM_" + errorBody.getScimStatus());
5687
}
57-
return errorBody;
5888
}
5989

6090
/**
6191
* The response is either a JSON response or a webpage. In the JSON case, it is parsed normally;
6292
* in the webpage case, the relevant error metadata is extracted from the response using a
6393
* heuristic-based approach.
6494
*/
65-
private static ApiErrorBody parseApiError(Response response) {
95+
private static Optional<ApiErrorBody> parseApiError(Response response) {
96+
InputStream in = response.getBody();
97+
if (in == null) {
98+
return Optional.empty();
99+
}
100+
101+
// Read the body now, so we can try to parse as JSON and then
102+
// fallback to old error handling logic.
103+
String body;
66104
try {
67-
InputStream in = response.getBody();
68-
if (in == null) {
69-
ApiErrorBody errorBody = new ApiErrorBody();
70-
errorBody.setMessage(
71-
String.format("Status response from server: %s", response.getStatus()));
72-
return errorBody;
73-
}
74-
75-
// Read the body now, so we can try to parse as JSON and then fallback to old error handling
76-
// logic.
77-
String body = IOUtils.toString(in, StandardCharsets.UTF_8);
78-
try {
79-
return MAPPER.readValue(body, ApiErrorBody.class);
80-
} catch (IOException e) {
81-
return parseUnknownError(response, body, e);
82-
}
105+
body = IOUtils.toString(in, StandardCharsets.UTF_8);
83106
} catch (IOException e) {
84107
throw new DatabricksException("Unable to read response body", e);
85108
}
109+
110+
if (Strings.isNullOrEmpty(body)) {
111+
return Optional.empty();
112+
}
113+
114+
try {
115+
return Optional.of(MAPPER.readValue(body, ApiErrorBody.class));
116+
} catch (IOException e) {
117+
return Optional.of(parseUnknownError(response, body, e));
118+
}
86119
}
87120

88121
private static ApiErrorBody parseUnknownError(Response response, String body, IOException err) {

‎databricks-sdk-java/src/test/java/com/databricks/sdk/core/ApiClientTest.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
import com.databricks.sdk.core.error.PrivateLinkValidationError;
77
import com.databricks.sdk.core.error.details.ErrorDetails;
88
import com.databricks.sdk.core.error.details.ErrorInfo;
9+
import com.databricks.sdk.core.error.platform.TooManyRequests;
910
import com.databricks.sdk.core.http.Request;
1011
import com.databricks.sdk.core.http.Response;
1112
import com.databricks.sdk.core.utils.FakeTimer;
@@ -235,8 +236,7 @@ void failAfterTooManyRetries() throws IOException {
235236
"Request GET /api/my/endpoint failed after 4 retries");
236237
assertInstanceOf(DatabricksError.class, exception.getCause());
237238
DatabricksError cause = (DatabricksError) exception.getCause();
238-
assertEquals(cause.getErrorCode(), "TOO_MANY_REQUESTS");
239-
assertEquals(cause.getStatusCode(), 429);
239+
assertInstanceOf(TooManyRequests.class, cause);
240240
}
241241

242242
@Test
@@ -430,6 +430,7 @@ void verifyNoRetryWhenRetriesDisabled() throws IOException {
430430
new Request("GET", req.getUri().getPath()), MyEndpointResponse.class));
431431

432432
assertEquals("TOO_MANY_REQUESTS", exception.getErrorCode());
433+
assertInstanceOf(TooManyRequests.class, exception);
433434
assertEquals(429, exception.getStatusCode());
434435
}
435436

0 commit comments

Comments
 (0)