L1nq0 commented on PR #9093:
URL: https://github.com/apache/storm/pull/9093#issuecomment-5695776676
@rzo1 Thanks for the review. Commit 971c768 addresses the points.
On the message text: agreed, the schedule contract is broader than the retry
limit, so the log no longer names a reason. It now reads:
```
The retry service will not retry message [{}]: the tuple will be acked and
its offset and later offsets may be committed even though the record was not
processed.
```
On rate limiting: I considered it and agree a plain ERROR is right here.
With the default maxRetries the branch never runs, and a finite limit means the
user opted into the loss. Each line also corresponds to a distinct lost record,
which is exactly what an operator needs to see when records are being given up
on; users who need suppression have the onMaxRetryReached hook and their
logging configuration.
On the ordering: testFailingTupleCompletesAckAfterRetryLimitIsMet now
installs a listener mock and verifies with InOrder, for each given-up tuple,
that onMaxRetryReached runs before the tuple is acked. The spout reports the
ack through the listener's onAck, so the pair pins the documented contract.
I left the debug line in KafkaSpoutRetryExponentialBackoff#schedule as is,
per your note.
--
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]
