[GitHub] [kafka] mjsax commented on a diff in pull request #13925: KAFKA-10199: Consider tasks in state updater when computing offset sums

["mjsax (via GitHub)" <gi...@apache.org>]

mjsax commented on code in PR #13925:
URL: https://github.com/apache/kafka/pull/13925#discussion_r1247273467


##########
streams/src/main/java/org/apache/kafka/streams/processor/internals/TaskManager.java:
##########
@@ -1141,25 +1141,30 @@ public Map<TaskId, Long> getTaskOffsetSums() {
         // Not all tasks will create directories, and there may be directories for tasks we don't currently own,
         // so we consider all tasks that are either owned or on disk. This includes stateless tasks, which should
         // just have an empty changelogOffsets map.
-        for (final TaskId id : union(HashSet::new, lockedTaskDirectories, tasks.allTaskIds())) {

Review Comment:
   It seems with the state updated enabled, `tasks` is actually only containing "running tasks". It seems appropriate the rename this variable to `runningTasks` (can also happen in a follow up PR).
   
   I am actually also wondering if we still need this `Tasks` container any longer to begin with? The purpose of the `Tasks` container was to simplify `TaskManager` that manages both active and standby tasks. With the state updated (from my understanding) the `TaskManager` only manages active tasks, while standby tasks will be owned by the state-updated-thread (would it still be useful for the state-updated-thread to use `Tasks` container, given that is also own active tasks as long as they are restoring?



-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org