squah-confluent commented on code in PR #22969:
URL: https://github.com/apache/kafka/pull/22969#discussion_r3750387956
##########
coordinator-common/src/main/java/org/apache/kafka/coordinator/common/runtime/CoordinatorRuntime.java:
##########
Review Comment:
The comment is indeed wrong and needs updating. Both succeeding and
reverting an empty batch are incorrect in different ways. Basically we need
state changes from non-replaying writes to be applied or reverted atomically
with the records. If we have an empty batch with state changes attached it
means we have separated them and that's wrong.
Regarding other paths:
There aren't any other happy paths that flush an empty batch. The non-atomic
flush later in the method will never do it because `hasRoomFor` returns true
when the batch is empty. Even without that, it's invalid to have a
non-replaying non-atomic write anyway, since we can't revert the correct part
of the in-memory updates when the write fails partially.
If we start considering unhappy paths, that's a huge can of worms. The only
way we can have an empty batch after an `append()` call is if it threw. Both
succeeding and failing an empty batch are incorrect in different situations. If
the empty batch is from a previous failed non-replaying write, succeeding the
batch would keep its in-memory changes without writing corresponding records.
Arguably we should add exception handling to `append()` to guarantee that it
never leaves an empty batch.
---
tl;dr I'm not keen on this _right now_ because it doesn't really fix the
unhappy paths and I want to avoid scope creeping this PR.
To close all the unhappy paths, ideally we would:
* Add exception handling to `append()` to never leave empty batches
* which lets us clean up `flushCurrentBatch()` to succeed empty batches,
ie. handle 0-record batches the same way as 1-record batches.
* Add exception handling to `CoordinatorWriteEvent` to revert in-memory
changes when an exception is thrown before appending records.
And even then, large non-replaying writes would still be bugged, since the
in-memory changes would be attached to any existing batch, but the records
would go in the next batch.
(Filed as KAFKA-20912)
--
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]
