Copilot commented on code in PR #7029:
URL: https://github.com/apache/shenyu/pull/7029#discussion_r3921655808
##########
shenyu-plugin/shenyu-plugin-fault-tolerance/shenyu-plugin-ratelimiter/src/main/java/org/apache/shenyu/plugin/ratelimiter/executor/RedisRateLimiter.java:
##########
@@ -60,18 +60,20 @@ public Mono<RateLimiterResponse> isAllowed(final String id,
final RateLimiterHan
List<String> keys = rateLimiterAlgorithm.getKeys(id);
List<String> scriptArgs = Stream.of(replenishRate, burstCapacity,
Instant.now().getEpochSecond(),
requestCount).map(String::valueOf).collect(Collectors.toList());
Flux<List<Long>> resultFlux =
Singleton.INST.get(ReactiveRedisTemplate.class).execute(script, keys,
scriptArgs);
- return resultFlux.onErrorResume(throwable ->
Flux.just(Arrays.asList(1L, -1L)))
+ // the error is absorbed right here, so callback/logging must happen
in this lambda, not in a downstream doOnError
+ return resultFlux
+ .onErrorResume(throwable -> {
+
rateLimiterAlgorithm.callback(rateLimiterAlgorithm.getScript(), keys,
scriptArgs);
+ LOG.error("Error occurred while judging if user is allowed
by RedisRateLimiter, fail-open and allow the request:{}",
throwable.getMessage());
+ return Flux.just(Arrays.asList(1L, -1L));
+ })
Review Comment:
In the `onErrorResume` handler, the error log only prints
`throwable.getMessage()` so the stack trace/cause chain is lost, and
`rateLimiterAlgorithm.getScript()` is redundantly re-fetched even though
`script` is already computed (and could theoretically differ or throw).
Consider logging the full Throwable and reusing the existing `script`; also
guard the callback so a callback exception doesn't turn the intended fail-open
path back into an error.
--
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]
