refactor: manage state associated with a resource in a single spot #1319
Conversation
|
|
||
| public interface RateLimiter { | ||
| public interface RateLimiter<S extends RateLimitState> { | ||
| interface RateLimitState { |
There was a problem hiding this comment.
Can't this be just an Object? Do we need an explicit interface for this.
I mean just use RateLimiter<S>
There was a problem hiding this comment.
Yeah, not sure either… Was thinking that perhaps we needed some some of marker interface to be able to trace where this state is used more easily than tracking Object… Will review.
There was a problem hiding this comment.
I think we should keep it this way so that it's more obvious what the associated state is for, even though technically Object would work just as well.
There was a problem hiding this comment.
Can't remember where but we had the same situation, where we decided to go with the object and don't use marker interfaces.
So I lean more to remove it, but I'm fine having that if you think it has benefits.
There was a problem hiding this comment.
It also has the advantage of having an interface already in case we need to somehow restrict what can be used…
There was a problem hiding this comment.
| @Override | ||
| public Optional<Duration> acquirePermission(ResourceID resourceID) { | ||
| if (limitForPeriod <= 0) { | ||
| public Optional<Duration> isLimited(RateLimitState rateLimitState) { |
There was a problem hiding this comment.
This is essentially where having a marker interface helps for documentation purposes as the alternative would be: isLimited(Object state) which wouldn't be as explicit. It's not a big deal but I think it's nicer this way.
|
Kudos, SonarCloud Quality Gate passed! |
This is a preliminary exploration to address #1314.
Thinking about this some more, this has additional advantages:
EventProcessormethods instead of retrieving it all the time from theResourceIDRateLimiterinterface