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

Fix X-RateLimit headers handling when using CompositeDelayFunction #1672

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

sklv-max
Copy link

@sklv-max sklv-max commented Nov 20, 2024

  1. Filtering out default durations in the CompositeDelayFunction.
  2. Returning default duration from the CompositeDelayFunction instead of null.

Description

Since RateLimitResetDelayFunction and RetryAfterDelayFunction are returning Duration.ofMinutes(-1) instead of null as the default value since Riptide 4, it is needed to check not only for non-null values but also if there are any non-default delays in the CompositeDelayFunction.

Motivation and Context

  1. In Riptide 4 the default value in RateLimitResetDelayFunction and RetryAfterDelayFunction was changed from null to Duration.ofMinutes(-1) link
  2. Failsafe 2.x.x used both negative duration and null as default values (link)
  3. Failsafe 3.x.x mentions only negative values as default values (link). If null is returned from delay function, it leads to NPE in the Failsafe.

As Riptide 4, if CompositeDelayFunction consists of two functions (A and B) and A returns default value and B returns a positive meaningful value, the default value would be taken since it is not null.

The issue is opened here.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

@sklv-max sklv-max force-pushed the fix_x_ratelimit_headers_handling branch from 055fa29 to d6c07d3 Compare November 20, 2024 10:55
@fatroom fatroom added the bugfix Bug fixes and patches, e.g. fixing of a production issue that is affecting the customer experience. label Nov 20, 2024
@fatroom
Copy link
Member

fatroom commented Nov 20, 2024

@sklv-max please verify your gpg key and sign commit: https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

2. Returning default duration from the CompositeDelayFunction instead of null
@sklv-max sklv-max force-pushed the fix_x_ratelimit_headers_handling branch from d6c07d3 to d44f0e0 Compare November 20, 2024 13:11
@sklv-max
Copy link
Author

@sklv-max please verify your gpg key and sign commit: https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

Thank you for pointing out, have done that.

@fatroom
Copy link
Member

fatroom commented Nov 20, 2024

👍

1 similar comment
@puncleV
Copy link
Contributor

puncleV commented Nov 21, 2024

👍

@fatroom fatroom merged commit 83e2365 into zalando:main Nov 21, 2024
4 checks passed
@fatroom
Copy link
Member

fatroom commented Nov 21, 2024

Thank you for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Bug fixes and patches, e.g. fixing of a production issue that is affecting the customer experience.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants