rzo1 commented on PR #9093:
URL: https://github.com/apache/storm/pull/9093#issuecomment-5694113933
The level change itself is fine. A few points before it goes in.
The message text is hardcoded to "Reached maximum number of retries", but
the contract of `KafkaSpoutRetryService#schedule` is broader:
> It may also indicate that the message should not be retried, in which case
the message will not be scheduled.
A custom retry service that declines a message for some other reason now
emits an ERROR stating the wrong reason. That was tolerable at debug level, at
error level it is not. Please reword so it holds for any implementation, e.g.
"The retry service will not retry message [{}]: the tuple will be acked and
...".
Second, this logs once per given-up tuple. With the default `maxRetries` of
`Integer.MAX_VALUE` it never fires, so most deployments are unaffected, but
with a low `maxRetries` and a failing downstream bolt it writes one ERROR per
tuple. We added rate limiting to `DeserializingConnectionCallback` for exactly
that pattern in #9076. I am inclined to keep a plain ERROR here, since the user
opted into the loss by setting a limit, but say whether you considered it.
Third, the javadoc you added to `KafkaTupleListener#onMaxRetryReached` now
states that the tuple is acked right after the callback, and nothing verifies
that ordering.
`KafkaSpoutRetryLimitTest#testFailingTupleCompletesAckAfterRetryLimitIsMet`
already sets up a zero-retry service, so an `InOrder` check on
`onMaxRetryReached` followed by the ack would pin the contract you are
documenting.
Minor: `KafkaSpoutRetryExponentialBackoff#schedule` still logs the same
event at debug. Not worth changing, just noting the duplication.
--
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]
