-
Notifications
You must be signed in to change notification settings - Fork 13.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-10199: Shutdown with new remove operation in state updater #15894
KAFKA-10199: Shutdown with new remove operation in state updater #15894
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @cadonna.
clearInputQueue(); | ||
updatingTasks.clear(); | ||
pausedTasks.clear(); | ||
changelogReader.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why was this added? Where was it executed before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before, it was executed in removeUpdatingAndPausedTasks()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored this code a bit.
public void start() { | ||
if (stateUpdaterThread == null) { | ||
if (!restoredActiveTasks.isEmpty() || !exceptionsAndFailedTasks.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you add this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that since we allow to restart the state updater, we should verify that the state updater starts with clean queue to avoid invalid states coming from a past run of the state updater. At the moment, we never restart the state updater but I thought having clear invariants might be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why we'd even allow restarting the state updater. but we don't need to fix it now
@@ -954,8 +954,7 @@ private void recycleTaskFromStateUpdater(final Task task, | |||
} | |||
} | |||
|
|||
/** Returns true if the task closed clean */ | |||
private boolean closeTaskClean(final Task task, | |||
private void closeTaskClean(final Task task, | |||
final Set<Task> tasksToCloseDirty, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
b306901
to
80d2983
Compare
Uses the new remove operation of the state updater that returns a future to shutdown the task manager.
80d2983
to
e732de3
Compare
The removed verification is based on a wrong assumption about when standby tasks are promoted. This is related to KIP-988.
…che#15894) Uses the new remove operation of the state updater that returns a future to shutdown the task manager. Reviewer: Lucas Brutschy <lbrutschy@confluent.io>
…che#15894) Uses the new remove operation of the state updater that returns a future to shutdown the task manager. Reviewer: Lucas Brutschy <lbrutschy@confluent.io>
Uses the new remove operation of the state updater that returns
a future to shutdown the task manager.
Committer Checklist (excluded from commit message)