dongnuo123 commented on code in PR #22969: URL: https://github.com/apache/kafka/pull/22969#discussion_r3738786887
########## coordinator-common/src/main/java/org/apache/kafka/coordinator/common/runtime/CoordinatorRuntime.java: ########## Review Comment: Yeah i think it makes sense. The only concern might be that if we really have another path that can result in the same bug with flushing empty batch, directly freeing the batch might cause divergence between the memory and logs. eg the [scenario](https://issues.apache.org/jira/browse/KAFKA-18200) we wanted to fix with this ` if (currentBatch.builder.numRecords() == 0) {` branch. If it's a downgrade and the in-mem state has been updated and serializeKey/serializeValue throws, the exception is not caught, no record is written so we could end up in an empty batch with in-mem change. In this case we still need the fail the batch to revert the state. Also I wonder what if we don't end up with an empty batch because there's already something in the batch when the serialization error happens? Looks like no revert will be triggered and we will have memory changed while no changes in log? -- 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]
