HY-love-sleep commented on code in PR #7166:
URL: https://github.com/apache/shenyu/pull/7166#discussion_r4078260141
##########
shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-token-limiter/src/main/java/org/apache/shenyu/plugin/ai/token/limiter/AiTokenLimiterPlugin.java:
##########
@@ -169,10 +170,11 @@ private String getCacheKey(final ServerWebExchange
exchange, final String tokenL
}
private void recordTokensUsage(final ReactiveRedisTemplate
reactiveRedisTemplate, final String cacheKey, final Long tokens, final Long
windowSeconds) {
- // Record token usage with expiration
- reactiveRedisTemplate.opsForValue()
- .increment(cacheKey, tokens)
- .flatMap(currentValue ->
reactiveRedisTemplate.expire(cacheKey, Duration.ofSeconds(windowSeconds)))
+ // The counter is given its window when it is created: re-issuing the
expiration after every increment
+ // would push the window forward, so a sustained traffic would never
reset the token budget.
+ final ReactiveValueOperations valueOperations =
reactiveRedisTemplate.opsForValue();
+ valueOperations.setIfAbsent(cacheKey, 0L,
Duration.ofSeconds(windowSeconds))
Review Comment:
Fixed in `021431b01`. You were right about the mechanism, and I reproduced
it outside the mocks before changing anything: `new
StringRedisSerializer().serialize(0L)` throws `java.lang.ClassCastException:
class java.lang.Long cannot be cast to class java.lang.String`, while `"0"`
serializes to `[48]`.
The value written for a new counter is the String `"0"` now, and I removed
the raw type that let it compile in the first place: `doExecute`, `isAllowed`
and `recordTokensUsage` all declare `ReactiveRedisTemplate<String, String>`, so
handing a `Long` to `setIfAbsent` is a compile error again. Typing `isAllowed`
forced one adjacent change there: `defaultIfEmpty(0L)` on a `Mono<String>` is
`defaultIfEmpty("0")` now, and the redundant `.toString()` before
`Long.parseLong` is gone.
The mocks in the test are typed `ReactiveRedisTemplate<String, String>` /
`ReactiveValueOperations<String, String>` as you suggested, and there is also a
test that pushes the value through the real `stringSerializationContext()`
value pair.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
