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

Jump to bottom

#1590 Use correct interval for request counting #1592

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

#1590 Use correct interval for request counting #1592

wants to merge 5 commits into from
+41 −24

Conversation

sergio-str
Copy link

@sergio-str sergio-str commented Aug 1, 2022 •
edited by raman-m

Fixes #1590

Incrementing request counter if request time is in the interval
[FirstRequestTime, FirstRequestTime + PeriodTimespan] seems is incorrect. PeriodTimespan is a "ban" period, not a period for request counting. In case of PeriodTimespan interval significantly exceeds Period, Rate Limiting feature counts number of requests longer than expected. It leads to false rate limit exceeding error.

Proposed Changes

Increment request counter when request time within interval
[FirstRequestTime, FirstRequestTime + RateLimitingSettingPeriod] respectively to the Rate Limiting docs.

@sergio-str sergio-str closed this Aug 4, 2022
@sergio-str sergio-str reopened this Aug 4, 2022
@raman-m
Copy link
Member

raman-m commented May 11, 2023

Hi Sergii!

Thanks for the solution and interest in the project!

I've resolved one merge conflict. And pipelines are green.
Could we discuss the issue #1590 first plz?

@raman-m raman-m added needs validation Issue has not been replicated or verified yet in progress Someone is working on the issue. Could be someone on the team or off. waiting Waiting for answer to question or feedback from issue raiser labels May 11, 2023
@raman-m raman-m self-requested a review May 11, 2023 13:16
@sergio-str
Copy link
Author

Yes, sure.

@raman-m raman-m linked an issue May 11, 2023 that may be closed by this pull request
Rate Limiting works incorrectly if PeriodTimespan value is greater than Period #1590
Open
@raman-m raman-m changed the title Use correct interval for request counting #1590 Use correct interval for request counting May 20, 2023
@raman-m
Copy link
Member

raman-m commented May 29, 2023

Sergii,

Now I see that at least your changes don't break current features.
But we have 2 parents in commit Merge branch 'develop' into Rate_linit_counting_interval_fix__issue_#….
I'm not a fan of merge commits with two parents after conflicts resolution, because they make non-linear history in base branch.
You need to put your commits on the top commit from base:develop!

Could I ask you to re-create PR please? Unfortunately your feature branch is broken. Your forked develop branch must be the same as develop branch in source repo!

  • Just backup files please with new changes.
  • Remove current fork, and make new fork to be sure you have the same history as in source repo having all commits up to date
    Or pull down all base:develop commits
  • Don't apply changes in develop, just keep it updated via pull operation always, just for future usage
  • Create new feature branch from develop one, for example, feat/1590
  • Apply new changes from files backup.
  • Create new PR
  • Do not forget to link both PRs (old and new ones) for tracking purposes

As soon as you've created new PR, this PR will be closed as duplicate.
Thanks!

@sergio-str
Copy link
Author

Hello Raman.
Sorry for significant delay, many things required my attention.
First, let me please confirm that you title and descr update is correct.
As for my implementation, I have a small doubt concerning timing calculation. I need to recheck them, and, perhaps, extend existing tests. Unfortunately I could not promise I will have solved it fast, but I will do all my best to finish with code changes in 2-3 weeks.
Cheers
S.

@raman-m
Copy link
Member

raman-m commented May 30, 2023

No worries, Sergii!
Take some time to finish the solution!
Tom will wait for your fix for a couple of years. 🤣 It's a joke!

@raman-m raman-m added needs feedback Issue is waiting on feedback before acceptance and removed in progress Someone is working on the issue. Could be someone on the team or off. labels May 30, 2023
@raman-m raman-m removed their request for review May 30, 2023 15:55
@raman-m
Copy link
Member

raman-m commented Jun 8, 2023 •
edited

Sergii,
Please merge PR 1 to update your repo!
After merging let's try to find the best option to update feature branch which has some merge conflicts for rebase-operation...
Thank you!

@raman-m
Copy link
Member

raman-m commented Jun 20, 2023

sergio-str commented on May 29, 2023:

Unfortunately I could not promise I will have solved it fast, but I will do all my best to finish with code changes in 2-3 weeks.

Well... 3 weeks have been passed.
Any progress on that?
May I help you with this PR by some pair programming?

@raman-m raman-m force-pushed the Rate_linit_counting_interval_fix__issue_#1590 branch from a26e1ac to 8910ba1 Compare September 7, 2023 16:18
@raman-m raman-m mentioned this pull request Sep 7, 2023
Open
@raman-m raman-m added bug Identified as a potential bug and removed waiting Waiting for answer to question or feedback from issue raiser labels Sep 7, 2023
@raman-m
Copy link
Member

raman-m commented Sep 7, 2023

The feature branch has been rebased onto ThreeMammals:develop!
Welcome to code review!
I guess we can develop and review the code simultaneously.

@raman-m raman-m added this to the Annual 2023 milestone Mar 5, 2024
@raman-m raman-m changed the base branch from develop to release/24.0 March 5, 2024 09:51
@raman-m raman-m force-pushed the Rate_linit_counting_interval_fix__issue_#1590 branch from 047790a to 6f4221c Compare April 7, 2024 13:34
@raman-m raman-m force-pushed the Rate_linit_counting_interval_fix__issue_#1590 branch 2 times, most recently from 4e08b8f to ef68e12 Compare April 19, 2024 17:44
@raman-m raman-m self-assigned this Apr 23, 2024
@raman-m raman-m added Mar-Apr'24 March-April 2024 release and removed 2023 Annual 2023 release labels Apr 23, 2024
@raman-m raman-m modified the milestones: Annual 2023, March-April'24 Apr 23, 2024
@raman-m raman-m added high High priority and removed needs validation Issue has not been replicated or verified yet needs feedback Issue is waiting on feedback before acceptance labels Apr 23, 2024
@raman-m raman-m changed the base branch from release/24.0 to develop April 23, 2024 16:18
@raman-m raman-m force-pushed the Rate_linit_counting_interval_fix__issue_#1590 branch from ef68e12 to 5b9744c Compare April 23, 2024 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raman-m raman-m raman-m requested changes

@RaynaldM RaynaldM RaynaldM approved these changes

Requested changes must be addressed to merge this pull request.

Assignees

@raman-m raman-m

@sergio-str sergio-str

Labels
bug Identified as a potential bug high High priority Mar-Apr'24 March-April 2024 release Rate Limiting Ocelot feature: Rate Limiting
Projects
None yet
Milestone
March-April'24
Development

Successfully merging this pull request may close these issues.

Rate Limiting works incorrectly if PeriodTimespan value is greater than Period
3 participants
@sergio-str @raman-m @RaynaldM