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 all commits
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
29 changes: 23 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 @@ -115,13 +117,28 @@ public boolean isSuccess() {
}

private void checkRetryable() {
Gson gson = new Gson();
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)) {
if (httpStatus == HTTP_STATUS_TOO_MANY_REQUESTS) {
isRetryable = true;
this.responseBody =
Copy link

Choose a reason for hiding this comment

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

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.

String.format(
JSON_ERROR_RESPONSE_TEMPLATE,
"Too many requests to ServiceNow API - decrease concurrent requests");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"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.");

}
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.

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 (Throwable t) {
// Any exception
isRetryable = false;
this.responseBody = String.format(JSON_ERROR_RESPONSE_TEMPLATE, t.getMessage());
}
}

Expand Down