search

platinummonkey / go-concurrency-limits

Go implementation of Netflix/concurrency-limits
Apache License 2.0
115 stars 20 forks source link

Limits do not get updated when all listeners are dropped #141

Closed jakeng-grabtaxi closed 1 month ago

jakeng-grabtaxi commented 8 months ago

Hi there,

I was taking a look at this library after learning about how Netflix controls concurrency. I noticed that in DefaultListener.OnDropped, the sample is added to the window but the concurrency limit is not updated (https://github.com/platinummonkey/go-concurrency-limits/blob/master/limiter/default.go#L100). The limit seems to only be updated in DefaultListener.OnSuccess. Whereas in the Java equivalent, onDropped (https://github.com/Netflix/concurrency-limits/blob/master/concurrency-limits-core/src/main/java/com/netflix/concurrency/limits/limiter/AbstractLimiter.java#L116) updates both the window and the limit.

I was wondering whats the reason for this difference and could it potentially lead to a situation where if the error rate spikes rapidly, such that that all requests are failing that the limits will never get updated.

Thank you for this great library!

jakeng-grabtaxi commented 3 months ago

Hi @platinummonkey , gently bumping this. I am happy to provide a PR to change this, but I wanted to run it through you first. Thanks.

platinummonkey commented 3 months ago

hey! Sorry definitely feel free to submit a PR anytime!