-
Notifications
You must be signed in to change notification settings - Fork 15.4k
KAFKA-15045: (KIP-924 pt. 13) AssignmentError calculation added #16114
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -38,6 +38,9 @@ | |||||
| import org.apache.kafka.streams.errors.TaskAssignmentException; | ||||||
| import org.apache.kafka.streams.processor.TaskId; | ||||||
| import org.apache.kafka.streams.processor.assignment.ApplicationState; | ||||||
| import org.apache.kafka.streams.processor.assignment.KafkaStreamsAssignment; | ||||||
| import org.apache.kafka.streams.processor.assignment.KafkaStreamsState; | ||||||
| import org.apache.kafka.streams.processor.assignment.TaskAssignor.AssignmentError; | ||||||
| import org.apache.kafka.streams.processor.assignment.TaskInfo; | ||||||
| import org.apache.kafka.streams.processor.assignment.ProcessId; | ||||||
| import org.apache.kafka.streams.processor.assignment.TaskAssignor.TaskAssignment; | ||||||
|
|
@@ -1522,6 +1525,72 @@ private void maybeScheduleFollowupRebalance(final long encodedNextScheduledRebal | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| private AssignmentError validateTaskAssignment(final ApplicationState applicationState, | ||||||
| final TaskAssignment taskAssignment) { | ||||||
| final Collection<KafkaStreamsAssignment> assignments = taskAssignment.assignment(); | ||||||
| final Set<TaskId> activeTasksInOutput = new HashSet<>(); | ||||||
| final Set<TaskId> standbyTasksInOutput = new HashSet<>(); | ||||||
| for (final KafkaStreamsAssignment assignment : assignments) { | ||||||
| final Set<TaskId> tasksForAssignment = new HashSet<>(); | ||||||
| for (final KafkaStreamsAssignment.AssignedTask task : assignment.assignment()) { | ||||||
| if (activeTasksInOutput.contains(task.id()) && task.type() == KafkaStreamsAssignment.AssignedTask.Type.ACTIVE) { | ||||||
| log.error("Assignment is invalid: an active task was assigned multiple times: {}", task.id()); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nice to include the ProcessId of both clients. We'll need to change
Suggested change
|
||||||
| return AssignmentError.ACTIVE_TASK_ASSIGNED_MULTIPLE_TIMES; | ||||||
|
ableegoldman marked this conversation as resolved.
|
||||||
| } | ||||||
|
|
||||||
| if (tasksForAssignment.contains(task.id())) { | ||||||
| log.error("Assignment is invalid: both an active and standby assignment of a task were assigned to the same client: {}", task.id()); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: we should include the client id as well
Suggested change
|
||||||
| return AssignmentError.ACTIVE_AND_STANDBY_TASK_ASSIGNED_TO_SAME_KAFKASTREAMS; | ||||||
| } | ||||||
|
|
||||||
| tasksForAssignment.add(task.id()); | ||||||
| if (task.type() == KafkaStreamsAssignment.AssignedTask.Type.ACTIVE) { | ||||||
| activeTasksInOutput.add(task.id()); | ||||||
| } else { | ||||||
| standbyTasksInOutput.add(task.id()); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| for (final TaskInfo task : applicationState.allTasks()) { | ||||||
| if (!task.isStateful() && standbyTasksInOutput.contains(task.id())) { | ||||||
| log.error("Assignment is invalid: a standby task was found for a stateless task: {}", task.id()); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, would be nice to log which client it was assigned to (which means turning
Suggested change
|
||||||
| return AssignmentError.INVALID_STANDBY_TASK; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| final Map<ProcessId, KafkaStreamsState> clientStates = applicationState.kafkaStreamsStates(false); | ||||||
| final Set<ProcessId> clientsInOutput = assignments.stream().map(KafkaStreamsAssignment::processId) | ||||||
| .collect(Collectors.toSet()); | ||||||
| for (final Map.Entry<ProcessId, KafkaStreamsState> entry : clientStates.entrySet()) { | ||||||
| final ProcessId processIdInInput = entry.getKey(); | ||||||
| if (!clientsInOutput.contains(processIdInInput)) { | ||||||
| log.error("Assignment is invalid: one of the clients has no assignment: {}", processIdInInput.id()); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit (clarify "KafkaStreams client" since "client" as a term is so massively overloaded):
Suggested change
|
||||||
| return AssignmentError.MISSING_PROCESS_ID; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| for (final ProcessId processIdInOutput : clientsInOutput) { | ||||||
| if (!clientStates.containsKey(processIdInOutput)) { | ||||||
| log.error("Assignment is invalid: one of the clients in the assignment is unknown: {}", processIdInOutput.id()); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| return AssignmentError.UNKNOWN_PROCESS_ID; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| final Set<TaskId> taskIdsInInput = applicationState.allTasks().stream().map(TaskInfo::id) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, changing the Then we can remove this line altogether and move the |
||||||
| .collect(Collectors.toSet()); | ||||||
| for (final KafkaStreamsAssignment assignment : assignments) { | ||||||
| for (final KafkaStreamsAssignment.AssignedTask task : assignment.assignment()) { | ||||||
| if (!taskIdsInInput.contains(task.id())) { | ||||||
| log.error("Assignment is invalid: one of the tasks in the assignment is unknown: {}", task.id()); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| return AssignmentError.UNKNOWN_TASK_ID; | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| return AssignmentError.NONE; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Verify that this client's host info was included in the map returned in the assignment, and trigger a | ||||||
| * rebalance if not. This may be necessary when using static membership, as a rejoining client will be handed | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.