INT-4497: Add rate limiting advice for handler #2544
Conversation
JIRA: https://jira.spring.io/browse/INT-4497 Add Rate Limiting advice for handler which supports Rate, MaxDelay
| private final long maxDelayInMillis; | ||
| private final long oneTimeUnitInMillis; | ||
| private final AtomicLongArray queue; | ||
| private final int rate; |
There was a problem hiding this comment.
This style is not readable.
Would be better to surround each class member to the blank lines.
Thanks
|
|
||
| /** | ||
| * Create an instance of RequestHandlerRateLimiterAdvice with the provided arguments. | ||
| * |
There was a problem hiding this comment.
We prefer do not have blank lines in the method Javadocs: https://github.com/spring-projects/spring-framework/wiki/Code-Style#javadoc-formatting
| * @param maxDelay the max allowable delay in time units. Use -1 for no max delay. | ||
| * @param timeUnit the time unit. | ||
| */ | ||
| public RequestHandlerRateLimiterAdvice(int rate, int maxDelay, TimeUnit timeUnit) { |
There was a problem hiding this comment.
Can't we use a Duration instead ?
| this.currentIndex = new AtomicInteger(0); | ||
| this.maxDelayInMillis = timeUnit.toMillis(maxDelay); | ||
| this.oneTimeUnitInMillis = timeUnit.toMillis(1); | ||
| this.queue = new AtomicLongArray(rate); |
There was a problem hiding this comment.
A queue is not a valuable name for the variable, especially when it is an object property.
Please, come up with more self-explanatory name.
Right, now it isn't clear what is its purpose.
| Assert.notNull(timeUnit, "Timeunit should be non null"); | ||
| this.currentIndex = new AtomicInteger(0); | ||
| this.maxDelayInMillis = timeUnit.toMillis(maxDelay); | ||
| this.oneTimeUnitInMillis = timeUnit.toMillis(1); |
There was a problem hiding this comment.
What is the magic number 1 millisecond?
Why do we need it?
Well, even if it is important, why can't we initialize it during definition?
What is the point to overhead a ctor body?
The same is about a currentIndex initialization here.
| /** | ||
| * An exception thrown when the max delay is triggered for rate limiter. | ||
| */ | ||
| public static final class RateLimiterMaxDelayException extends MessagingException { |
There was a problem hiding this comment.
Well, actually exceptions are named after actions: ResourceNotFound, TransactionTimeout etc.
I'm not sure in the algorithm here yet, so I can't come up with the better name...
MaxDelayExceededException ?
The RateLimiter is already implied with the wrapping class. 🤷♂️
|
|
||
| private void delayIfNecessary(Object target, Message<?> message) throws InterruptedException { | ||
| long timeToWait = -1; | ||
| while (timeToWait == -1) { |
There was a problem hiding this comment.
I believe there should be a way for fail fast for a single call or as we already know per rate.
I mean if we still in the rate we doin't need to sleep or even calculate System.currentTimeMillis() at all.
I need to understand algorithm better, because it looks like I have more questions, than review requires 😢
| @@ -49,11 +49,12 @@ For chains that produce a reply, every child element can be advised. | |||
| [[advice-classes]] | |||
| ==== Provided Advice Classes | |||
|
|
|||
| In addition to providing the general mechanism to apply AOP advice classes, Spring Integration provides three standard advice classes: | |||
| In addition to providing the general mechanism to apply AOP advice classes, Spring Integration provides four standard advice classes: | |||
There was a problem hiding this comment.
Well, let's fix it for something like these standard advice classes to cover all the possible future additions 😄
| The Rate Limiter advice allows you to ensure that an endpoints does not get overloaded with requests. | ||
| When the RateLimit is breached the request will go in a blocked state. | ||
|
|
||
| A typical use case for this advice might be an external service provider not allowing more than n number of request per minute. |
There was a problem hiding this comment.
I believe n must be wrapped to code to emphasize it a bit.
| ==== | ||
| [source,xml] | ||
| ---- | ||
| <int:service-activator input-channel="input" ref="failer" method="service"> |
There was a problem hiding this comment.
Can we have a Java sample as well, please?
|
I suggest to revise our own algorithm here in favor of well-known and stable solution in the Resilience4J: http://resilience4j.github.io/resilience4j/#_ratelimiter In the future we may also reconsider our Circuit Breaker implementation to be based on their solution as well: http://resilience4j.github.io/resilience4j/#_circuitbreaker Any thoughts? Thanks |
|
Sure; makes sense. |
JIRA: https://jira.spring.io/browse/INT-4497
Add Rate Limiting advice for a handler which supports Rate, MaxDelay