-
Notifications
You must be signed in to change notification settings - Fork 99
fix: owners set UNDER_LIMIT status when Remaining=0 #212
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
if err != nil { | ||
gm.log.WithError(err).Errorf("while broadcasting update to peers for: '%s'", rl.HashKey()) | ||
continue | ||
} | ||
status := misleadingStatus | ||
if misleadingStatus.Remaining == 0 { | ||
status.Status = Status_OVER_LIMIT | ||
} | ||
// Build an UpdatePeerGlobalsReq | ||
req.Globals = append(req.Globals, &UpdatePeerGlobal{ | ||
Algorithm: rl.Algorithm, | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should