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

[MRESOLVER-396] Back off on too many requests #326

Merged
merged 8 commits into from
Sep 4, 2023

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Aug 19, 2023

Provide a way to "back off", and lower the request pace if remote claims "too many requests"


https://issues.apache.org/jira/projects/MRESOLVER/issues/MRESOLVER-396

Provide a way to "back off", and lower the request pace
if remote claims "too many requests"

---

https://issues.apache.org/jira/projects/MRESOLVER/issues/MRESOLVER-396
@cstamas cstamas self-assigned this Aug 19, 2023
@cstamas cstamas requested a review from michael-o August 20, 2023 20:15
@michael-o michael-o removed their request for review August 28, 2023 14:03
@cstamas cstamas added this to the 1.9.16 milestone Sep 2, 2023
@cstamas cstamas marked this pull request as ready for review September 2, 2023 10:10
@cstamas cstamas requested review from gnodet and kwin September 4, 2023 09:33
* exponential backoff
* Retry-After header
* Maximum of retryInterval, when request should be rather failed
Copy link
Member

@kwin kwin left a comment

Choose a reason for hiding this comment

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

LGTM, just minor remarks about documentation

src/site/markdown/configuration.md Outdated Show resolved Hide resolved
src/site/markdown/configuration.md Outdated Show resolved Hide resolved
@@ -43,8 +43,11 @@ Option | Type | Description | Default Value | Supports Repo ID Suffix
`aether.connector.http.preemptiveAuth` | boolean | Should HTTP client use preemptive-authentication for all HTTP verbs (works only w/ BASIC). By default is disabled, as it is considered less secure. | `false` | yes
`aether.connector.http.preemptivePutAuth` | boolean | Should HTTP client use preemptive-authentication for HTTP PUTs only (works only w/ BASIC). By default is enabled (same as Wagon). | `true` | yes
`aether.connector.http.retryHandler.count` | int | The maximum number of times a request to a remote HTTP server should be retried in case of an error. | `3` | yes
`aether.connector.http.retryHandler.interval` | long | The initial retry interval if server responds with "too many requests". Is multiplied by 1, 2,.. on each try. | `5000` | yes
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`aether.connector.http.retryHandler.interval` | long | The initial retry interval if server responds with "too many requests". Is multiplied by 1, 2,.. on each try. | `5000` | yes
`aether.connector.http.retryHandler.interval` | long | The initial retry interval if server responds with "too many requests". Previous value is multiplied by 2 on each retry. | `5000` | yes

Copy link
Member Author

Choose a reason for hiding this comment

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

No, is not, initial value is multiplied with 1, 2, 3...

Copy link
Member

Choose a reason for hiding this comment

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

Isn’t this what my suggestion says as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think not, you proposed "exponential", where you have some sort of loop like this:

retryInterval = initial
loop:
  retryInterval = retryInterval * counter

For input 1, 2, 3 and initial=5000 this produces 5000, 10000, 30000

What I did is slightly different:

retryInterval;
loop:
  retryInterval = initial * counter

For input 1,2,3 and initial=5000 this produces 5000, 10000, 15000 etc

IMHO, "real exponential" does not really make sense here: your build is time sensitive, if you are forced to wait (exponentially) for server as it is overloaded, then its better to make your IT env/vendor fix it 😄

Copy link
Member

Choose a reason for hiding this comment

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

sorry, you are right. Let's leave it like you proposed.

Copy link
Contributor

@gnodet gnodet left a comment

Choose a reason for hiding this comment

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

LGTM, though a comment to explain the usage of the ThreadLocal would be welcomed imho.

@cstamas cstamas merged commit 3c073bc into apache:master Sep 4, 2023
7 checks passed
@cstamas cstamas deleted the MRESOLVER-396 branch September 4, 2023 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants