Nikita-Shupletsov commented on code in PR #21365:
URL: https://github.com/apache/kafka/pull/21365#discussion_r2744794087
##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/TaskManager.java:
##########
@@ -1523,7 +1532,7 @@ private Collection<Task> tryCloseCleanActiveTasks(final
Collection<Task> activeT
final boolean clean,
final
AtomicReference<RuntimeException> firstException) {
if (!clean) {
- return activeTaskIterable();
+ return activeTasksToClose;
Review Comment:
the reason why I decided to change this was that if we follow the call:
*
https://github.com/apache/kafka/blob/trunk/streams/src/main/java/org/apache/kafka/streams/processor/internals/TaskManager.java#L1405,
here we pass two lists of tasks to close. the active ones used to accept
activeTaskIterable as well, but it was changed at some point.
* then we get into closeAndCleanUpTasks, then into tryCloseCleanActiveTasks,
tryCloseCleanStandbyTasks
* at this point we check if we even need to try to close them cleanly. if
not `Returns the set of active tasks that must be closed dirty`
* at this point it makes sense to return the list we received, not a
different list. because it's a potential place for a bug: we ask the method to
close one set of tasks, but it closes a different set of tasks.
even without my change it's already a bit incorrect, because we take the
active tasks from the task registry and from the state updater:
https://github.com/apache/kafka/blob/trunk/streams/src/main/java/org/apache/kafka/streams/processor/internals/TaskManager.java#L1708-L1709,
which isn't really a big deal, because we handle the state updater before this
call, so there should be nothing there.
but if it changes, it will be pretty hard to notice that slight change in
the behavior.
on top of that, as you mentioned, this list doesn't contain the pending
tasks. so if we keep it as is, when we shutdown dirtily, we will not close them.
--
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]
