allthingssecurity commented on code in PR #27989:
URL: https://github.com/apache/camel/pull/27989#discussion_r4301873149
##########
core/camel-core-processor/src/main/java/org/apache/camel/processor/aggregate/AggregateProcessor.java:
##########
@@ -1580,6 +1580,24 @@ private void onEviction(Listener.Type type, String key,
String exchangeId) {
if (type != Listener.Type.Evict) {
return;
}
+ try {
+ doOnEviction(key, exchangeId);
+ } catch (Exception e) {
+ // the entry is no longer in the timeout map, and the timeout
map ignores errors of its listeners, so
+ // without a new entry a group the repository could not read
or remove would never time out (an entry
+ // that a new message for the key has put in the meantime is
kept)
+ long delay = getCompletionTimeoutCheckerInterval();
+ LOG.warn("Error completing the group with correlation key: {}
by its completion timeout,"
+ + " will try again in {} ms. Caused by: {}",
+ key, delay, e.getMessage());
Review Comment:
Thanks, fixed in c5cd806dd354: the WARN now logs `e.toString()`, so it names
the exception type. The new test `testRepeatedFailuresOfAGroupLogOneWarn`
checks that the line contains `java.lang.IllegalStateException: Simulated
repository failure in get`.
_Claude Code on behalf of allthingssecurity_
##########
core/camel-core-processor/src/main/java/org/apache/camel/processor/aggregate/AggregateProcessor.java:
##########
@@ -1580,6 +1580,24 @@ private void onEviction(Listener.Type type, String key,
String exchangeId) {
if (type != Listener.Type.Evict) {
return;
}
+ try {
+ doOnEviction(key, exchangeId);
+ } catch (Exception e) {
+ // the entry is no longer in the timeout map, and the timeout
map ignores errors of its listeners, so
+ // without a new entry a group the repository could not read
or remove would never time out (an entry
+ // that a new message for the key has put in the meantime is
kept)
+ long delay = getCompletionTimeoutCheckerInterval();
+ LOG.warn("Error completing the group with correlation key: {}
by its completion timeout,"
+ + " will try again in {} ms. Caused by: {}",
+ key, delay, e.getMessage());
+ LOG.debug("Error completing the group with correlation key: {}
by its completion timeout", key, e);
+ if (isRunAllowed()) {
+ putIfAbsent(key, exchangeId, delay);
Review Comment:
Thanks. In c5cd806dd354 I took the first option. The first failure of a key
is logged at WARN ("...will try again every 50 ms (further failures for this
key are logged at DEBUG)"), and the next tries of that key are logged at DEBUG
with the stack trace. The timeout map keeps the keys it has warned about in a
set. A key leaves the set when its completion works, or when a new message puts
or removes its timeout entry, so a later failure is logged at WARN again. The
set only holds keys that have a retry entry.
New test `testRepeatedFailuresOfAGroupLogOneWarn`: the repository fails
three reads of the group, the group is still completed, and exactly one WARN is
logged for the key. Without the change, it fails with 3 WARNs. Aggregate tests
in camel-core: 148 run, 0 failures.
I did not add the back-off. It would also shorten the time spent under the
aggregator lock with a slow store, but then even a short outage would delay the
completion by a whole `completionTimeout`. I have noted it as a follow-up.
_Claude Code on behalf of allthingssecurity_
--
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]
