Skip to content
This repository has been archived by the owner on Apr 19, 2024. It is now read-only.

fix: owners set UNDER_LIMIT status when Remaining=0 #212

Closed
wants to merge 2 commits into from

Conversation

miparnisari
Copy link
Contributor

@miparnisari miparnisari commented Jan 22, 2024

This is a copy of #207 but with a potential fix in place.

Closes #208

@miparnisari miparnisari requested a review from Baliedge as a code owner January 22, 2024 02:55
@@ -224,11 +224,15 @@ func (gm *globalManager) broadcastPeers(ctx context.Context, updates map[string]
SetBehavior(&rl.Behavior, Behavior_GLOBAL, false)
rl.Hits = 0

status, err := gm.instance.getLocalRateLimit(ctx, rl)
misleadingStatus, err := gm.instance.getLocalRateLimit(ctx, rl)
Copy link
Contributor

Choose a reason for hiding this comment

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

did you confirm with the test that this returns with the correct result? I saw various places where the "fix" could be made at the time but it's hard to know if there is any knock on effects elsewhere without knowing the codebase more thoroughly.

Hopefully it is this simple :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've only stared at the codebase for a couple of hours haha. As I mentioned here #208 (comment) the other place where I thought we could fix this is in algorithms.go.. but I am not comfortable changing that code because it doesn't have any unit tests 😬

{
Name: "test_global",
UniqueKey: "account:12345",
Algorithm: guber.Algorithm_LEAKY_BUCKET,
Copy link
Contributor

Choose a reason for hiding this comment

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

@miparnisari - I think we will want to extend the test case to cover both algorithms perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we should

@thrawn01
Copy link
Contributor

Hi all! I think I have a fix for this in PR #216, please check it out!

@miparnisari miparnisari closed this Feb 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected GetRateLimits result from non-owning peer when using Global behaviour
3 participants