mjsax commented on code in PR #23794:
URL: https://github.com/apache/kafka/pull/23794#discussion_r4418798883
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/streams/AssignmentRefinerImpl.java:
##########
@@ -236,7 +236,9 @@ static Map<String, ProcessLoad> indexProcessLoad(
* process: (1) either the old owner of the task keeps running it, or (2)
caught-up copy which we temporarily
* promote as active is running it.
*
- * <p>Only an active-running or caught-up copy is ever promoted.
+ * <p>Only an active-running or caught-up copy is ever promoted, with one
exception: the standby task of a task
+ * that no member runs as an active task, on a member that does not report
its restore progress, see
Review Comment:
```suggestion
* no member holds as an active task, on a member that does not report
its restore progress, see
```
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/streams/AssignmentRefinerImpl.java:
##########
@@ -236,7 +236,9 @@ static Map<String, ProcessLoad> indexProcessLoad(
* process: (1) either the old owner of the task keeps running it, or (2)
caught-up copy which we temporarily
* promote as active is running it.
*
- * <p>Only an active-running or caught-up copy is ever promoted.
+ * <p>Only an active-running or caught-up copy is ever promoted, with one
exception: the standby task of a task
Review Comment:
```suggestion
* <p>Only an active-running or caught-up copy is ever promoted, with
one exception: the standby task
```
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/streams/AssignmentRefinerImpl.java:
##########
@@ -371,6 +392,45 @@ private static Optional<String> bestCopyToPromote(
.map(TaskCopy::memberId);
}
+ /**
+ * Picks a standby task to run a task that no member runs as an active
task, while the target owner warms it up,
+ * even though the standby's holder does not report how far it has
restored: the holder heartbeats with version 0,
+ * or has not heartbeated since the coordinator loaded the group.
+ *
+ * <p>Without this, the target owner would restore the task from scratch
while nobody runs it. On the standby's
+ * holder, the task runs sooner: a standby that is behind catches up
before it processes, which it would have to
+ * do as a standby anyway, and one that is close processes right away.
Once the target owner's warm-up task has
+ * caught up, the task moves there and the holder keeps it as a standby
task.
+ *
+ * <p>It applies only if no member runs the task as an active task, the
target owner's process holds no copy of
+ * the task, which the target owner would otherwise continue from, and the
target assignment keeps the standby
+ * task on its holder. If several standby tasks qualify, the one on the
least loaded process is picked.
+ *
+ * @param targetProcessId The process of the target owner, or {@code null}
if the target owner has left the group.
+ */
Review Comment:
```suggestion
/**
* The standby holder of the task to promote although its lag is
unknown, because the holder is not
* {@link WarmupSupport#SUPPORTED}. Only if no member holds the task as
an active task, the target owner's process
* holds no copy of the task, and the target assignment keeps the
standby task on its holder; among several, the
* least loaded process wins. A standby task that is behind restores
before it processes, which it has to do as a
* standby task anyway.
*
* @param targetProcessId The process of the target owner, or {@code
null} if the target owner has left the group.
*/
```
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/streams/AssignmentRefinerImpl.java:
##########
@@ -371,6 +392,45 @@ private static Optional<String> bestCopyToPromote(
.map(TaskCopy::memberId);
}
+ /**
+ * Picks a standby task to run a task that no member runs as an active
task, while the target owner warms it up,
+ * even though the standby's holder does not report how far it has
restored: the holder heartbeats with version 0,
+ * or has not heartbeated since the coordinator loaded the group.
+ *
+ * <p>Without this, the target owner would restore the task from scratch
while nobody runs it. On the standby's
+ * holder, the task runs sooner: a standby that is behind catches up
before it processes, which it would have to
+ * do as a standby anyway, and one that is close processes right away.
Once the target owner's warm-up task has
+ * caught up, the task moves there and the holder keeps it as a standby
task.
+ *
+ * <p>It applies only if no member runs the task as an active task, the
target owner's process holds no copy of
+ * the task, which the target owner would otherwise continue from, and the
target assignment keeps the standby
+ * task on its holder. If several standby tasks qualify, the one on the
least loaded process is picked.
+ *
+ * @param targetProcessId The process of the target owner, or {@code null}
if the target owner has left the group.
+ */
Review Comment:
Don't think the current comment is correct.
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/streams/AssignmentRefinerImpl.java:
##########
@@ -1250,9 +1310,11 @@ record TaskCopy(String memberId, String processId,
TaskRole role, boolean caught
* The task being migrated.
* @param currentOwner
* The member the task runs on for now: the member holding it as an
active task, or, when that member is
- * too far behind to run it or there is none, the caught-up copy
holder promoted to run it. When the target
- * assignment names a member the group no longer has, it can also
be a holder that is still restoring the
- * task, which is better than taking the task away from a restore
that is part-way through.
+ * too far behind to run it or there is none, the caught-up copy
holder promoted to run it. For a task that
+ * no member runs as an active task, it can also be a standby
holder that does not report its restore
Review Comment:
```suggestion
* no member holds as an active task, it can also be a standby
holder that does not report its restore
```
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/streams/AssignmentRefinerImpl.java:
##########
@@ -236,7 +236,9 @@ static Map<String, ProcessLoad> indexProcessLoad(
* process: (1) either the old owner of the task keeps running it, or (2)
caught-up copy which we temporarily
* promote as active is running it.
*
- * <p>Only an active-running or caught-up copy is ever promoted.
+ * <p>Only an active-running or caught-up copy is ever promoted, with one
exception: the standby task of a task
+ * that no member runs as an active task, on a member that does not report
its restore progress, see
Review Comment:
"holds" is more generic, ie, covers both active-running and
active-restoring, while "runs" might indicate active-running
--
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]
