mjsax commented on code in PR #23454:
URL: https://github.com/apache/kafka/pull/23454#discussion_r4002322300
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/streams/AssignmentRefinerImpl.java:
##########
@@ -268,12 +323,24 @@ static TaskDecisions analyzeTasks(
* is dropped when the task closes, and no hand-over of a running task
between threads of one process exists to
* carry it across. Worse, the lag that made the task look ready was
measured on the member that is about to
* close, so for an in-memory store it says nothing about what the
incoming member then has to restore. This
- * predicate cannot fix that; it would take a client-side cross-thread
task hand-over. The broker cannot even see
- * the difference, because the topology metadata carries changelog topics
but not how a store is backed.
+ * predicate cannot fix that; it would take a client-side cross-thread
task hand-over
+ * (https://issues.apache.org/jira/browse/KAFKA-21090). The broker cannot
even see the difference, because the
+ * topology metadata carries changelog topics but not how a store is
backed.
+ *
+ * <p>The damage is bounded, though, because <b>the refiner never creates
one of those two paths -- it only ever
+ * inherits them.</b> Every warm-up it plants sits on the target owner
itself, so every migration it warms ends in
+ * the in-place promotion, which is warm for every store type. It even
pays to keep that true: where the
+ * destination process holds a copy of the task only on a <em>sibling</em>
of the target owner, the budget pass
+ * spends a slot to move that copy onto the target owner rather than
borrow it where it sits. So the only way to
+ * reach one of the two cold paths is through this predicate granting the
task outright -- nothing was warmed, and
+ * the layout was already there when the refiner looked.
*
- * <p>What bounds the damage is that a warm-up task the refiner plants
always targets the target owner itself, so
- * every migration the refiner stages resolves through the in-place
promotion. The other paths arise only out of a
- * layout the refiner inherited.
+ * <p>Note that includes a copy on a sibling member that is
<em>already</em> caught up: the task is granted here,
+ * in this step, before the budget pass ever sees the migration, so
nothing gets the chance to move that copy onto
+ * the target owner first. Doing so would spend a slot to buy an in-place
promotion -- which is worth it for an
+ * in-memory store and pure waste for a store that persists to disk, since
that one reopens warm from the state
+ * directory anyway. The broker cannot tell the two apart, so this grants
immediately and converges fast. It is a
+ * deliberate boundary rather than an oversight, and the design document
carries it as an open question.
Review Comment:
This last paragraph atm merely documents the current state of the art --
it's the same as we do in "classic", and it does not affect persistent stores
because they survive the hand-over.
However, it seems to be a problem for in-memory stores, so it's an open
question if we want to address it right now, or maybe only later, or accept
as-is (because "classic" has the same issue, so it's not a regression).
Personally, I tend to think we should address right way (if we agree, I
would do a follow up PR for it).
--
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]
