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.

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

Create a configurable API retry strategy #261

Open
U3R1YXJ0 opened this issue Feb 18, 2021 · 11 comments
Open

Create a configurable API retry strategy #261

U3R1YXJ0 opened this issue Feb 18, 2021 · 11 comments
Labels
Status: Completed Describe solution, include code snippets, indicate version # Type: Question

Comments

@U3R1YXJ0
Copy link

SDK you're using (please complete the following information):
4.1.4

Is your feature request related to a problem? Please describe.
We regularly get unspecified "com.xero.api.XeroServerErrorException: Internal Server Errors" on API calls. In the last 2 weeks we've had 13 instances. They are not related to a specific call - when we manually retry we normally succeed second time.

This probably wouldn't matter for many atomic use cases, but we have functionality that runs a series of calls over the space of 30 minutes. If we get an internal server error we stop, because there is an unknown problem. We're are now looking at creating a retry solution within our client.

Describe the solution you'd like
The Xero client could include a default and configurable retry strategy for failed API calls.

Describe alternatives you've considered
We're planning on building our own solution.

Additional context
Ideally the Xero API would be more specific about any issues. At the moment we could retry on all internal server errors, and our experience suggest this is sensible. However if there was some specific information at Xero that could indicate if the client should retry again or not, that would be ideal.

@lancedfr
Copy link
Contributor

@U3R1YXJ0, this is already supported by the SDK. All you need to do is override the default http client in the Xero API class with one that is configured with retries. For example you can configure an Apache http client with a retry policy, and set the Apache http client onto the respective Xero API class.

Once I get a free moment I will send you a example to get you going.

@U3R1YXJ0
Copy link
Author

Hi Lance, awesome, that sounds great! Thanks, a example will be really useful.

@lancedfr
Copy link
Contributor

lancedfr commented Feb 19, 2021

Hi Stuart, please see an example below, refactor to your requirement. We configure the AccountingApi instance as a managed bean so its not created every request.

  void someOperation() {
      ApiClient apiClient = new ApiClient();
      apiClient.setHttpTransport(new ApacheHttpTransport(newRetryHttpClient()));
      //ConnectionTimeout defaults to 20 seconds if not set
      apiClient.setConnectionTimeout(CONNECTION_TIMEOUT);
      //ReadTimeout defaults to 20 seconds if not set
      apiClient.setReadTimeout(READ_TIMEOUT);
      AccountingApi accountingApi = AccountingApi.getInstance(apiClient);
      //or AssetApi.getInstance(apiClient), etc
  }

  DefaultHttpClient newRetryHttpClient() {
    DefaultHttpClient defaultHttpClient = new DefaultHttpClient();
    defaultHttpClient.setHttpRequestRetryHandler(new DefaultHttpRequestRetryHandler(RETRY_COUNT, false));
    return defaultHttpClient;
  }

One last thing to note, the Xero SDK uses google-http-client to facilitate http connections. The google-http-client can be configured to use Apache http client, pure Java http client (or others like okhttp, not sure), the point is you can configure whichever underlaying http client you want with its retry policy, connection managers, SSL config, http schema config, etc. You not forced to use Apache http client like my above example does.

@U3R1YXJ0
Copy link
Author

Ok, got it, that's great. Yes, I remember configuring the google client now (it was quite a while ago). Thanks for the example, we will give that a go.

@SidneyAllen SidneyAllen added Status: Completed Describe solution, include code snippets, indicate version # Type: Question labels Feb 19, 2021
@SidneyAllen
Copy link
Contributor

@lancedfr - thanks for helping on this issue.

I'll leave this open so I can add this info to the README.

@U3R1YXJ0
Copy link
Author

U3R1YXJ0 commented Feb 23, 2021

@lancedfr Just to mention that we've found DefaultHttpClient is deprecated in favour of HttpClient. The Xero SDK currently uses google-api-client 1.23.0. In this version, the ApacheHttpTransport client is not compatible with HttpClient, but we think it is in later versions of google-api-client.

We're going to stick with the deprecated DefaultHttpClient for now.

Perhaps you could update google-api-client to a more recent version? 1.23.0 is from October 2017. Looks like this would make integration with ApacheHttpTransport easier whilst using the recomended HttpClient.

EDIT: Just realised you don't work for Xero @lancedfr! So the suggestion is for @SidneyAllen

@U3R1YXJ0
Copy link
Author

We declared a more recent version of google-api-client as a dependency in our application POM to override the default version specified in the Xero SDK, and that seems to work fine.

@lancedfr
Copy link
Contributor

lancedfr commented Feb 23, 2021

Correct, I do not work for Xero. I only contribute to the SDK and did some work on the "first oauth 1" SDK 😁

I wanted to create a request to update the google-api-client to a newer version as a result of the deprecated dependency on Apache http code. I speak under correction I think the latest version of google-api-client still depends on the deprecated Apache code??? If so, we'll need to update to google-http-client-apache-v2, which I think is a breaking change for those who already use Apache Http client.

EDIT: just realised you said you declared a more recent version of google-api-client, meaning its probably not dependant on deprecated Apache http code. Maybe we can create a separate request to update to the latest version of google-api-client 🙂

@SidneyAllen
Copy link
Contributor

Hey @lancedfr and @U3R1YXJ0

I recently did a spike to see what the impact of updating the google-api-client from 1.23.0 to 1.31.2

I found the methods for interacting with Xero API endpoints all functioned properly, but the methods used during our authentication flow required modification.

For example in the Callback servlet under 1.23.0 we use the following code.

final JsonFactory JSON_FACTORY = new JacksonFactory();

DataStoreFactory DATA_STORE_FACTORY = new MemoryDataStoreFactory();

AuthorizationCodeFlow flow = new AuthorizationCodeFlow.Builder(BearerToken.authorizationHeaderAccessMethod(),
                    HTTP_TRANSPORT, JSON_FACTORY, new GenericUrl(TOKEN_SERVER_URL),
                    new ClientParametersAuthentication(clientId, clientSecret), clientId, AUTHORIZATION_SERVER_URL)
                    .setScopes(scopeList).setDataStoreFactory(DATA_STORE_FACTORY).build();

TokenResponse tokenResponse = flow.newTokenRequest(authResponseCode).setRedirectUri(redirectURI).execute();

Under version 1.31.2 JacksonFactory(); has been deprecated and the callback code uses GsonFactory

TokenResponse tokenResponse = new AuthorizationCodeTokenRequest(new NetHttpTransport(), new GsonFactory(),
        			 new GenericUrl(TOKEN_SERVER_URL), clientId)
        			 	.setRedirectUri(redirectURI)
        			 	.setCode(authResponseCode)
        			 	.setScopes(scopeList)
        			 	.setGrantType("authorization_code")
        			 	.setClientAuthentication(new ClientParametersAuthentication(clientId, clientSecret))
        			 	.execute();

The code is 1.31.2 is cleaner and will be better as we move into the future but from what I can tell this is a breaking change and will require a major version update from 4.x to 5.x. I was holding off as I wanted to get the Files API support added in 4.7 of the SDK. The Files API work is complete now.

Any objections or support of this move to 1.31.2 for google-api-client?

@lancedfr
Copy link
Contributor

Hi @SidneyAllen, I support the move. I do not think anyone is in need of this upgrade, with that said I'd say it's low priority and it can for Files API work (or even longer for that matter).

PS. I believe #261 can be resolved as retry is supported in the SDK. Let me know if you want me to resolve this one and open a new feature request for the google-api-client upgrade?

@U3R1YXJ0
Copy link
Author

U3R1YXJ0 commented Mar 1, 2021

@SidneyAllen We also support the move. I also agree with @lancedfr that there is no hurry as individual clients can upgrade themselves by specifying a more recent version in their application POMs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Completed Describe solution, include code snippets, indicate version # Type: Question
Projects
None yet
Development

No branches or pull requests

3 participants