mjsax commented on PR #22864:
URL: https://github.com/apache/kafka/pull/22864#issuecomment-5287700537
@arnabnandy7 -- Thanks for the PR. Can you rebase to resolve merge conflicts?
Also a few minor follow ups (from Claude):
```
1. The invariant isn't enforced at two sites that can empty updatingTasks
The PR states the invariant as "when there are no updating tasks, the reader
is in ACTIVE_RESTORING", but two paths still violate it:
- addToExceptionsAndFailedTasksThenRemoveFromUpdatingTasks (line 412-414)
keeps if (exceptionAndTask.task().isActive()) around the call. A failing
standby that was the last updating task leaves the reader in STANDBY_UPDATING
with updatingTasks empty. The PR deliberately dropped that same guard in
removeUpdatingTask and pauseTask, so this reads as an oversight rather than a
decision.
- addToExceptionsAndFailedTasksThenClearUpdatingAndPausedTasks (line 440)
clears both maps and never calls the helper. Reachable with the thread still
alive via handleStreamsExceptionWithoutTask, which doesn't set isRunning =
false.
Severity: no crash results. The new isRestoringActive() guard prevents the
ISE from either path, and the state self-heals at the next removal that does
call the helper. What's left is the documented invariant being transiently
false and standby changelogs staying resumed on the restore consumer with
nothing updating. Worth fixing for consistency with the PR's own stated
contract — not a blocker on crash grounds.
2. Three test call-count churns come from a guaranteed no-op
shouldRestoreSingleActiveStatefulTask 1→2,
shouldRestoreMultipleActiveStatefulTasks 3→4, shouldResumeActiveStatefulTask
2→3. All three are the new updatingTasks.isEmpty() → enforceRestoreActive()
firing when the last active task completes — at which point the reader is
already ACTIVE_RESTORING, so the call is a guaranteed no-op
(enforceRestoreActive is internally guarded). Making the branch symmetric with
the other one:
if (updatingTasks.isEmpty() && !changelogReader.isRestoringActive()) {
keeps behaviour identical, drops three unrelated verify-count changes out of
the diff, and stops those tests being coupled to an incidental no-op.
3. The regression is caught only indirectly
Without the fix, shouldResumeStandbyTask fails after 30 seconds with "Did
not get all updating task within the given timeout! Expected: [StandbyTask],
actual: []". The real cause is only visible buried in captured stdout. Worth an
explicit test for the reported sequence asserting the reader state directly.
After the rebase there's a much better assertion available: KAFKA-20721
added stateUpdater.fatalException(), so the test can assert
assertEquals(Optional.empty(), stateUpdater.fatalException()) and fail fast
naming the actual cause.
```
--
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]
