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(plugin): RL instances sync to the same DB at same rate #12003

Merged
merged 1 commit into from Nov 17, 2023

Conversation

StarlightIbuki
Copy link
Contributor

@StarlightIbuki StarlightIbuki commented Nov 13, 2023

Summary

All rate-limiting plugin instance syncs with the same plugin config, that is the very first config got hit by a request, and they all sync with the same rate. Even a config update won't change the DB to be synced.

The timer will sync not just the same instance's counters but all counters in the same DB. This is a compromise given the emergency and we prefer simplicity over correctness for this behavior.

Checklist

  • The Pull Request has tests(the test is covered by adding a new for loop to the integration test)
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md

Full changelog

  • The counter table is split with DB;
  • Timers are created when a request hits;
  • The sync_rate is guaranteed with limited running timers and timer delay
  • Cover the case in the integration test by "with_sync_rate"

Issue reference

Fix KAG-2904

@StarlightIbuki StarlightIbuki mentioned this pull request Nov 14, 2023
2 tasks
and same rate

All rate-limiting plugin instance syncs with the same plugin config, that is the very first config got hit by a request, and they all sync with the same rate. Even a config update won't change the DB to be synced.

Fix KAG-2904

Co-authored-by: samugi <samuele@konghq.com>
Copy link
Contributor

@samugi samugi 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 a small question on the test

@windmgc windmgc merged commit a355d01 into master Nov 17, 2023
24 checks passed
@windmgc windmgc deleted the fix/rl-sync-share-timer branch November 17, 2023 03:43
hanshuebner pushed a commit that referenced this pull request Nov 20, 2023
All rate-limiting plugin instance syncs with the same plugin config, that is the very first config got hit by a request, and they all sync with the same rate. Even a config update won't change the DB to be synced.

The timer will sync not just the same instance's counters but all counters in the same DB. This is a compromise given the emergency and we prefer simplicity over correctness for this behavior.

Full changelog
- The counter table is split with DB;
- Timers are created when a request hits;
- The sync_rate is guaranteed with limited running timers and timer delay
- Cover the case in the integration test by "with_sync_rate"

Fix KAG-2904

Co-authored-by: samugi <samuele@konghq.com>
windmgc pushed a commit that referenced this pull request Jan 24, 2024
All rate-limiting plugin instance syncs with the same plugin config, that is the very first config got hit by a request, and they all sync with the same rate. Even a config update won't change the DB to be synced.

The timer will sync not just the same instance's counters but all counters in the same DB. This is a compromise given the emergency and we prefer simplicity over correctness for this behavior.

Full changelog
- The counter table is split with DB;
- Timers are created when a request hits;
- The sync_rate is guaranteed with limited running timers and timer delay
- Cover the case in the integration test by "with_sync_rate"

Fix KAG-2904

Co-authored-by: samugi <samuele@konghq.com>
windmgc pushed a commit that referenced this pull request Mar 7, 2024
All rate-limiting plugin instance syncs with the same plugin config, that is the very first config got hit by a request, and they all sync with the same rate. Even a config update won't change the DB to be synced.

The timer will sync not just the same instance's counters but all counters in the same DB. This is a compromise given the emergency and we prefer simplicity over correctness for this behavior.

Full changelog
- The counter table is split with DB;
- Timers are created when a request hits;
- The sync_rate is guaranteed with limited running timers and timer delay
- Cover the case in the integration test by "with_sync_rate"

Fix KAG-2904

Co-authored-by: samugi <samuele@konghq.com>
windmgc pushed a commit that referenced this pull request Mar 8, 2024
All rate-limiting plugin instance syncs with the same plugin config, that is the very first config got hit by a request, and they all sync with the same rate. Even a config update won't change the DB to be synced.

The timer will sync not just the same instance's counters but all counters in the same DB. This is a compromise given the emergency and we prefer simplicity over correctness for this behavior.

Full changelog
- The counter table is split with DB;
- Timers are created when a request hits;
- The sync_rate is guaranteed with limited running timers and timer delay
- Cover the case in the integration test by "with_sync_rate"

Fix KAG-2904

Co-authored-by: samugi <samuele@konghq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants