Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix error thrown by connector when 429 response code is received #63

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Fix error thrown by connector when 429 response code is received #63

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
871dd18
Fix error thrown by connector when 429 response code is received
saimukkamala Oct 16, 2023
55415a0
Fix error thrown by connector when 429 response code is received
saimukkamala Oct 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/main/java/io/cdap/plugin/servicenow/apiclient/RetryableException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,9 @@ public class RetryableException extends RuntimeException {
public RetryableException() {
super();
}

public RetryableException(String message) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong alignment

super(message);
}

}
10 changes: 8 additions & 2 deletions src/main/java/io/cdap/plugin/servicenow/apiclient/ServiceNowTableAPIClientImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,9 +132,15 @@ public List<Map<String, String>> fetchTableRecords(String tableName, SourceValue
apiResponse = executeGet(requestBuilder.build());
if (!apiResponse.isSuccess()) {
if (apiResponse.isRetryable()) {
throw new RetryableException();
throw new RetryableException(
String.format(
"Error in fetchTableRecords, http response code = %d, message = %s",
apiResponse.getHttpStatus(), apiResponse.getResponseBody()));
}
return Collections.emptyList();
throw new RuntimeException(
String.format(
"Error in fetchTableRecords, http response code = %d, message = %s",
apiResponse.getHttpStatus(), apiResponse.getResponseBody()));
}

return parseResponseToResultListOfMap(apiResponse.getResponseBody());
Expand Down
33 changes: 27 additions & 6 deletions src/main/java/io/cdap/plugin/servicenow/restapi/RestAPIResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.google.gson.Gson;
import com.google.gson.JsonObject;
import com.google.gson.JsonSyntaxException;
import io.cdap.plugin.servicenow.util.ServiceNowConstants;
import org.apache.http.Header;
import org.apache.http.HttpResponse;
Expand Down Expand Up @@ -48,6 +49,7 @@ public class RestAPIResponse {
" },\n" +
" \"status\": \"failure\"\n" +
"}";
private static final int HTTP_STATUS_TOO_MANY_REQUESTS = 429;
private int httpStatus;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All fields should be final.

private Map<String, String> headers;
private String responseBody;
Expand Down Expand Up @@ -116,12 +118,31 @@ public boolean isSuccess() {

private void checkRetryable() {
Gson gson = new Gson();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make it a constant, no need to new it everytime.

JsonObject jo = gson.fromJson(this.responseBody, JsonObject.class);
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)) {
isRetryable = true;
try {
JsonObject jo = gson.fromJson(this.responseBody, JsonObject.class);
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)) {
Comment on lines +130 to +135
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check if we require to handle null for the following in the responseBody json ?

  1. Missing ERROR node
  2. Missing MESSAGE node

isRetryable = true;
}
} catch (JsonSyntaxException e) {
// Response Body is not a json object - check status code for error
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we check the http status before parsing the body? Typically we should always check the status code before parsing the body.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Response body necessarily need not be json always (when 429 error is thrown -
// responseBody is HTML)
if (httpStatus == HTTP_STATUS_TOO_MANY_REQUESTS) {
isRetryable = true;
this.responseBody =
String.format(
JSON_ERROR_RESPONSE_TEMPLATE,
"Too many requests to ServiceNow API - decrease concurrent requests");
}
} catch (Throwable t) {
// Any other exception
isRetryable = false;
this.responseBody = String.format(JSON_ERROR_RESPONSE_TEMPLATE, t.getMessage());
}
}

Expand Down