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

Jump to bottom

Change backoff strategy to exponential for BitbucketCloudApiClient when rate-limited #841

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

Change backoff strategy to exponential for BitbucketCloudApiClient when rate-limited #841

wants to merge 2 commits into from
+8 −3

Conversation

thomas-boehm-tractive
Copy link
Contributor

Change backoff strategy to exponential for BitbucketCloudApiClient when rate-limited, as a fixed wait time of 5 seconds sends too many unnecessary requests that can be avoided.

Your checklist for this pull request

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side) and not your master branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or in Jenkins JIRA
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Did you provide a test-case? That demonstrates feature works or fixes the issue.
    • no- as I am unsure how to test this

Fixes #840

KalleOlaviNiemitalo
KalleOlaviNiemitalo reviewed Mar 28, 2024
View reviewed changes
src/main/java/com/cloudbees/jenkins/plugins/bitbucket/client/BitbucketCloudApiClient.java
LOGGER.log(Level.FINE, "Bitbucket Cloud API rate limit reached for {0}, sleeping for {1} sec then retry...", new Object[]{httpMethod.getURI(), sleepTime});
Thread.sleep(sleepTime);
retryCount++;
LOGGER.log(Level.FINE, "Retrying Bitbucket Cloud API request for {0}, retryCount={1}", new Object[]{httpMethod.getURI(), retryCount});
response = client.execute(host, httpMethod, requestContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this close the previous CloseableHttpResponse response? If so, that isn't a new bug though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As this is a different issue that is not really related to my changes, I guess it makes more sense to move this to a separate ticket and PR, if this causes issues. I think it doesn't really belong to this one.

@thomas-boehm-tractive thomas-boehm-tractive force-pushed the change-backoff-strategy-to-exponential branch from 094dd6d to 463cef9 Compare March 28, 2024 15:33
@lifeofguenter lifeofguenter added feature java Pull requests that update Java code labels Mar 28, 2024
@thomas-boehm-tractive thomas-boehm-tractive force-pushed the change-backoff-strategy-to-exponential branch from 463cef9 to 5f3a712 Compare March 29, 2024 08:58
@nfalco79
Copy link
Member

nfalco79 commented Apr 3, 2024

This PR is already covered by the #414 that have a long discussion.
There is also consideration in issue #831 and https://issues.jenkins.io/browse/JENKINS-64418

@thomas-boehm-tractive thomas-boehm-tractive force-pushed the change-backoff-strategy-to-exponential branch from 5f3a712 to 29811dc Compare May 23, 2024 06:28
@thomas-boehm-tractive
Copy link
Contributor Author

This PR is already covered by the #414 that have a long discussion.
There is also consideration in issue #831 and https://issues.jenkins.io/browse/JENKINS-64418

@nfalco79 does that mean I sould close this PR?

@nfalco79
Copy link
Member

PRs should refer to an issue with the reason of changes, in this cases there is one with details. For what I see other PR has an ongoing review with the old-old maintainers but was stucks. What I mean is that this PR should takes in consideration those changes, comments and the JIRA issue to completed.

@thomas-boehm-tractive thomas-boehm-tractive force-pushed the change-backoff-strategy-to-exponential branch from 29811dc to c1680c1 Compare July 15, 2024 13:50
@thomas-boehm-tractive thomas-boehm-tractive force-pushed the change-backoff-strategy-to-exponential branch from c1680c1 to a213bac Compare October 29, 2024 09:50
@nfalco79
Copy link
Member

nfalco79 commented Nov 21, 2024 •
edited
Loading

The rate limit impact both server and cloud instances.
So I would use the same logic for both clients and instead to use a custom implementation outside the Apache HTTP client implementation I would evaluate

HttpClientBuilder
    .create()
    .setRetryHandler(new bitbucketExponentialBackoffRetryHandler(...))

I found this implementation org.apache.http.impl.client.cache.ExponentialBackOffSchedulingStrategy maybe combined with org.apache.http.client.HttpRequestRetryHandler

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KalleOlaviNiemitalo KalleOlaviNiemitalo KalleOlaviNiemitalo left review comments

Assignees
No one assigned
Labels
feature java Pull requests that update Java code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Introduce exponential backoff strategy for BitbucketCloudApiClient for rate-limiting
4 participants
@thomas-boehm-tractive @nfalco79 @KalleOlaviNiemitalo @lifeofguenter