`CommonErrorHandler.isAckAfterHandle()` misleads developers in case of `false`

[ioanngolovko]

trafficstars

In what version(s) of Spring for Apache Kafka are you seeing this issue?

For example:

At least between 2.8.x and current

Describe the bug

CommonErrorHandler.isAckAfterHandle() misleads developers in case of false. It leads to pitfalls. Processing does not stop. Any successful processing of the message in the future will result in a commit.

To Reproduce

Set isAckAfterHandle to false

• Process multiple polls of bad messages (for example, message handling finishes with Runtime exception).
• Send good message to your topic and then offset of this message will be commited.

Expected behavior

Why do we need this isAckAfterHandle if the offset will be commited anyway? What about to remove it from CommonErrorHandler?

[sobychacko]

@ioanngolovko When it is false, the offset will not be committed after handling the error, and the exception will be thrown to the application. However, upon processing a successful record (in the next poll, for ex.), that record's offset will be committed, and the latest committed offset will be advanced for that consumer group, indicating that the earlier failed record was committed. What kind of pitfalls are you running into? I think this is the expected behavior.

[ioanngolovko]

@sobychacko If it's true - it will not be commited immediately, but will be put to acks cache. So I ask, what is the practical value of the method? It seems like rudiment.

[sobychacko]

Unless it is a manual immediate ack, the offsets will be put into a queue and then committed when all the records from the current poll are finished processing (the standard batch semantics). If the ack mode is manual_immediate, the commit should happen immediately.

[ioanngolovko]

It's not about the ackMode. It's about isAckAfterHandle. If it's false - developer doesn't want to commit anything at all. But commit appears little later with the first good message. And this is so illogical.

There are two ways to fix it:

1. Keep lag at bad point. But this will prevent further poll. Not elegant solution.
2. Remove this method from CommonErrorHandler due-to non-obviousness and dubious value.

[sobychacko]

@ioanngolovko I think this method was added for some slight optimization. It means that when this is false, we don't want to add this to the ack queue that will be committed, thus getting some slight performance improvement. But, eventually, when the next good record comes, its offset will be committed, which means that the log cursor for that topic partition is advanced. I think, therefore, that method was added as an optimization. If there are any unforeseen pitfalls, as you indicated, we are happy to triage this further if you give more context and details.

[ioanngolovko]

@sobychacko It's better to know exactly why it was added. Maybe we can live without it at all and reduce complexity.

[mminella]

Thank you for raising this issue. Unfortunately, as a project stewarded by US company, we are unable to accept contributions from Russian sources due to US law at this time. Thanks for your continued use of Spring.