ableegoldman commented on code in PR #19164:
URL: https://github.com/apache/kafka/pull/19164#discussion_r1988199982
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/TaskManager.java:
##########
@@ -1153,8 +1157,7 @@ void handleRevocation(final Collection<TopicPartition>
revokedPartitions) {
prepareCommitAndAddOffsetsToMap(revokedActiveTasks,
consumedOffsetsPerTask);
Review Comment:
Hm. Well my original reaction was the same as yours Matthias, ie that this
should only apply to EOSv2 as I thought we only committed the necessary tasks
with ALOS since he don't have to commit the entire transaction
However, now that you bring this up: I have a vague memory of making this
case before, but ultimately us agreeing to just commit every task under ALOS in
order to keep the logic from branching too much. I'm pretty sure it was a
"let's keep things simple and if we need to optimize further then we can stop
committing non-revoked tasks under ALOS"
I don't think it's become a problem yet so let's keep it as is and do the
same for EOS and ALOS.
Also FWIW I also had the same thought as Lucas, let's move this line into
the `if revokedTasksNeedCommit` condition as well, just because it makes the
logic easier to follow.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
