Fix error thrown by connector when 429 response code is received #63
Conversation
| isRetryable = true; | ||
| } | ||
| } catch (JsonSyntaxException e) { | ||
| // Response Body is not a json object - check status code for error |
There was a problem hiding this comment.
Why don't we check the http status before parsing the body? Typically we should always check the status code before parsing the body.
There was a problem hiding this comment.
Unfortunately servicenow doesn't have any public documentation with an exhaustive list of response codes. For now going with what we know and will be adding them in the future.
| @@ -26,5 +26,9 @@ public class RetryableException extends RuntimeException { | |||
| public RetryableException() { | |||
| super(); | |||
| } | |||
|
|
|||
| public RetryableException(String message) { | |||
| JSON_ERROR_RESPONSE_TEMPLATE, | ||
| "Too many requests to ServiceNow API - decrease concurrent requests"); | ||
| } | ||
| Gson gson = new Gson(); |
There was a problem hiding this comment.
Make it a constant, no need to new it everytime.
| isRetryable = true; | ||
| this.responseBody = |
There was a problem hiding this comment.
If we know the status is 429, why we need to create a json string, then parse it again just to get the status field as failure? Seem like we are overly complicating things here.
| @@ -48,6 +49,7 @@ public class RestAPIResponse { | |||
| " },\n" + | |||
| " \"status\": \"failure\"\n" + | |||
| "}"; | |||
| private static final int HTTP_STATUS_TOO_MANY_REQUESTS = 429; | |||
| private int httpStatus; | |||
| this.responseBody = | ||
| String.format( | ||
| JSON_ERROR_RESPONSE_TEMPLATE, | ||
| "Too many requests to ServiceNow API - decrease concurrent requests"); |
There was a problem hiding this comment.
| "Too many requests to ServiceNow API - decrease concurrent requests"); | |
| "ServiceNow Concurrent requests quota reached, please wait for the running requests to complete before triggering any new concurrent request."); |
| if (jo.get(ServiceNowConstants.STATUS) != null | ||
| && jo.get(ServiceNowConstants.STATUS).getAsString().equals(ServiceNowConstants.FAILURE) | ||
| && jo.getAsJsonObject(ServiceNowConstants.ERROR) | ||
| .get(ServiceNowConstants.MESSAGE) | ||
| .getAsString() | ||
| .contains(ServiceNowConstants.MAXIMUM_EXECUTION_TIME_EXCEEDED)) { |
There was a problem hiding this comment.
Can you check if we require to handle null for the following in the responseBody json ?
- Missing ERROR node
- Missing MESSAGE node
JsonSyntaxExceptionwhile serializing to Json. This PR addresses this fix and throws a meaningful error.ServiceNowTableAPIClientImpl.fetchTableRecordsreturns empty list which can cause data loss. Now this will throw an error instead of empty list.