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

Strange behavior of RateExceededError.RATE_EXCEEDED #121

Closed
dimasho opened this issue Jul 27, 2017 · 6 comments
Closed

Strange behavior of RateExceededError.RATE_EXCEEDED #121

dimasho opened this issue Jul 27, 2017 · 6 comments

Comments

@dimasho
Copy link

dimasho commented Jul 27, 2017

Hi,

I opened a thread in the AdWords group and they asked to post it here too (https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!topic/adwords-api/jlYlnR5BEaI)

Looks like in the new lib (3.5.0) RateExceededError.RATE_EXCEEDED exception is an instance of DetailedReportDownloadResponseException and not ApiException as before.

TTP Response Code: 400, Type: RateExceededError.RATE_EXCEEDED
at com.google.api.ads.adwords.lib.utils.v201705.DetailedReportDownloadResponseException$Builder.build(DetailedReportDownloadResponseException.java:35)
at com.google.api.ads.adwords.lib.utils.AdHocReportDownloadHelperImpl.handleResponse(AdHocReportDownloadHelperImpl.java:117)
at com.google.api.ads.adwords.lib.utils.AdHocReportDownloadHelperImpl.downloadReport(AdHocReportDownloadHelperImpl.java:60)
at sun.reflect.GeneratedMethodAccessor82.invoke(Unknown Source)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
at java.lang.reflect.Method.invoke(Unknown Source)
at com.google.api.ads.common.lib.utils.AdsUtilityInvocationHandler.handleInvocation(AdsUtilityInvocationHandler.java:46)
at com.google.common.reflect.AbstractInvocationHandler.invoke(AbstractInvocationHandler.java:84)
at com.sun.proxy.$Proxy33.downloadReport(Unknown Source)
at com.google.api.ads.adwords.lib.utils.AdHocReportDownloadHelper.downloadReport(AdHocReportDownloadHelper.java:46)
at com.google.api.ads.adwords.lib.utils.v201705.ReportDownloader.downloadReport(ReportDownloader.java:73)

@jradcliff
Copy link
Member

jradcliff commented Jul 27, 2017

Hi,

I saw on the forum post that you recently upgraded from 2.20.0 to 3.5.0. I just compared the source for reporting utilities between those versions, and I haven't found any related changes.

Are you 100% sure that in 2.20.0 you were getting a RateExceededError from reporting requests? I ask because AdHocReportDownloadHelperImpl has always only thrown one of the following exception types, none of which extend ApiException or RateExceededError:

In contrast, calls to non-reporting services such as ReportDefinitionService, CampaignService, etc., will throw RateExceededError.

Thanks,
Josh Radcliff, AdWords API Team

@jradcliff jradcliff self-assigned this Jul 27, 2017
@chenzhuo914
Copy link
Contributor

chenzhuo914 commented Jul 28, 2017

Josh is right. There shouldn't be such a behavior change.

The RateExceededError just wraps underlying AdWords API exception that is not related to rate limiting or is not retriable.

For reporting downloading, it never (in any previous versions) throws ApiException (see the list of possible exceptions from Josh), so there's no way for RateExceededError to contain an ApiException.

On the other hand, for other services (such as CampaignService), they may throw ApiException, therefore the RateExceededError may contain an ApiException. I guess what you saw (RateExceededError with an internal ApiException) was from an AdWords API service call, instead of report downloading.

@dimasho
Copy link
Author

dimasho commented Jul 28, 2017

Probably you're right, we have a common wrapper for all the API calls, both services and reporting. Apparently we catched this exception from some service call. Sorry about that.

This explanation raises another question - any reason not to throw ApiException in the report downloading as well? This kind of exception requires a special handling which on its side requires additional info like scope and recommended sleep duration.

Thanks

@chenzhuo914
Copy link
Contributor

This is a question to the client library, and I'm sure Josh can share some insights.

On the RateLimiter side, yes it differentiates report downloading and services, and handles rate limits (scope, wait after seconds) of two categories separately.

@jradcliff
Copy link
Member

Hi,

Having the report downloader throw ApiException is an interesting idea. It's a bit of a challenge for the following reasons.

No WSDL for reporting

Unlike with the SOAP services such as CampaignService, we do not have a WSDL that indicates all possible exceptions that could appear in a failed reporting response.

Where to introduce ApiException

Currently, the downloadReport method signature has

throws ReportException, ReportDownloadResponseException

and the method throws exceptions as follows (note that I just updated my
previous comment, which was not 100% correct):

  • ReportException
    • If the request fails due to an IOException, MalformedURLException, or AuthenticationException.
  • ReportDownloadResponseException
    • If the HTTP status code != 200 and
    • Reading the input stream from the response fails with an IOException.
  • DetailedReportDownloadResponseException - This is the only case where the response will contain an ApiException and its fields.
    • If the HTTP status code != 200 and
    • Reading the input stream from the response succeeds.

Therefore, it makes sense to modify DetailedReportDownloadResponseException so that it
extends ApiException instead of ReportDownloadResponseException. The only problem with that
change is that it will be a breaking change for anyone who is explicitly catching
ReportDownloadResponseException in a try/catch block.

The above can all be addressed, but it'll take some time to iron out all the details. I'll update
this issue as I make progress.

Cheers,
Josh Radcliff, AdWords API Team

@jradcliff jradcliff added P1 and removed P1 labels Dec 21, 2017
@jradcliff
Copy link
Member

Hi,

While reviewing open issues, I gave this one some more thought, and I recommend detecting rate limit errors using the approach employed by the RateLimiter extension, as shown in its ApiReportingRetryStrategy.

I mention this because upon further review, having DetailedReportDownloadResponseException extend ApiException looks like a non-starter. ☹️

The reporting classes in the library currently are not dependent on the SOAP toolkit (Axis or JAX-WS) or the toolkit-specific and API version-specific classes generated from WSDLs, such as:

This is a good thing, as it means users who are only interested in running reports can simply depend on the com.google.api-ads/ads-lib module, and not worry about Axis or JAX-WS dependencies and modules.

If we make DetailedReportDownloadResponseException extend ApiException, we'll need a separate DetailedReportDownloadResponseException class for JAX-WS and Axis and every version of the AdWords API, which would mean separate ReportDownloader classes for JAX-WS and Axis and every version of the AdWords API. In addition, any reporting-only users would have to bring in JAX-WS or Axis dependencies. All of this would be a breaking change for all users of the library, unfortunately.

Given the above, I'm going to close this issue, but feel free to reopen it if you have additional questions or ideas on this topic.

Thanks,
Josh, AdWords API Team

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants