Skip to content
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-14462; [10/N] Add TargetAssignmentBuilder #13637

Merged
merged 6 commits into from
May 2, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@
*/
public class ConsumerGroupMember {
/**
* A builder allowing to create a new member or update an
* existing one.
* A builder that facilitates the creation of a new member or the update of
* an existing one.
*
* Please refer to the javadoc of {{@link ConsumerGroupMember}} for the
* definition of the fields.
Expand Down Expand Up @@ -521,7 +521,7 @@ public Map<Uuid, Set<Integer>> partitionsPendingRevocation() {
}

/**
* @return The set of partitions awaiting assigning to the member.
* @return The set of partitions awaiting assignment to the member.
*/
public Map<Uuid, Set<Integer>> partitionsPendingAssignment() {
return partitionsPendingAssignment;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,327 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.apache.kafka.coordinator.group.consumer;

import org.apache.kafka.common.Uuid;
import org.apache.kafka.coordinator.group.Record;
import org.apache.kafka.coordinator.group.assignor.AssignmentMemberSpec;
import org.apache.kafka.coordinator.group.assignor.AssignmentSpec;
import org.apache.kafka.coordinator.group.assignor.AssignmentTopicMetadata;
import org.apache.kafka.coordinator.group.assignor.GroupAssignment;
import org.apache.kafka.coordinator.group.assignor.MemberAssignment;
import org.apache.kafka.coordinator.group.assignor.PartitionAssignor;
import org.apache.kafka.coordinator.group.assignor.PartitionAssignorException;

import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;

import static org.apache.kafka.coordinator.group.RecordHelpers.newTargetAssignmentEpochRecord;
import static org.apache.kafka.coordinator.group.RecordHelpers.newTargetAssignmentRecord;

/**
* Build a new Target Assignment based on the provided parameters. As a result,
* it yields the records that must be persisted to the log and the new member
* assignments as a map.
*
* Records are only created for members which have a new target assignment. If
* their assignment did not change, no new record is needed.
*
* When a member is deleted, it is assumed that its target assignment record
* is deleted as part of the member deletion process. In other words, this class
* does not yield a tombstone for removed members.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to add to the comment, but the assumption that the tombstone is created in the member deletion process as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the first sentence says: When a member is deleted, it is assumed that its target assignment record is deleted as part of the member deletion process.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read this, but I guess I just didn't assume that delete record = create tombstone. I guess that's a pretty easy conclusion to draw, it just didn't come to me right away.

*/
public class TargetAssignmentBuilder {
/**
* The assignment result returned by {{@link TargetAssignmentBuilder#build()}}.
*/
public static class TargetAssignmentResult {
/**
* The records that must be applied to the __consumer_offsets
* topics to persist the new target assignment.
*/
private final List<Record> records;

/**
* The new target assignment for all members.
*/
private final Map<String, Assignment> assignments;

TargetAssignmentResult(
List<org.apache.kafka.coordinator.group.Record> records,
Map<String, Assignment> assignments
) {
Objects.requireNonNull(records);
Objects.requireNonNull(assignments);
this.records = records;
this.assignments = assignments;
}

/**
* @return The records.
*/
public List<Record> records() {
return records;
}

/**
* @return The assignments.
*/
public Map<String, Assignment> assignments() {
return assignments;
}
}

/**
* The group id.
*/
private final String groupId;

/**
* The group epoch.
*/
private final int groupEpoch;

/**
* The partition assignor used to compute the assignment.
*/
private final PartitionAssignor assignor;

/**
* The members in the group.
*/
private Map<String, ConsumerGroupMember> members = Collections.emptyMap();

/**
* The subscription metadata.
*/
private Map<String, TopicMetadata> subscriptionMetadata = Collections.emptyMap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to confirm, this subscriptionMetadata holds topicId -> topic metadata and is for all members, i.e. a change in a member's subscription will also update this field

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.


/**
* The existing target assignment.
*/
private Map<String, Assignment> assignments = Collections.emptyMap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Jeff already touched this, but there is a small amount of confusion with this name (ie, between this and the result's assignment field).

It could be useful to use the same phrasing in the comments (ie, current vs. existing)


/**
* The members which have been updated or deleted. Deleted members
* are signaled by a null value.
*/
private final Map<String, ConsumerGroupMember> updatedMembers = new HashMap<>();

/**
* Constructs the object.
*
* @param groupId The group id.
* @param groupEpoch The group epoch to compute a target assignment for.
* @param assignor The assignor to use to compute the target assignment.
*/
public TargetAssignmentBuilder(
String groupId,
int groupEpoch,
PartitionAssignor assignor
) {
this.groupId = Objects.requireNonNull(groupId);
this.groupEpoch = groupEpoch;
this.assignor = Objects.requireNonNull(assignor);
}

/**
* Adds all the existing members.
*
* @param members The existing members in the consumer group.
* @return This object.
*/
public TargetAssignmentBuilder withMembers(
Map<String, ConsumerGroupMember> members
) {
this.members = members;
return this;
}

/**
* Adds the subscription metadata to use.
*
* @param subscriptionMetadata The subscription metadata.
* @return This object.
*/
public TargetAssignmentBuilder withSubscriptionMetadata(
Map<String, TopicMetadata> subscriptionMetadata
) {
this.subscriptionMetadata = subscriptionMetadata;
return this;
}

/**
* Adds the existing target assignments.
*
* @param assignments The existing target assignments.
* @return This object.
*/
public TargetAssignmentBuilder withTargetAssignments(
Map<String, Assignment> assignments
) {
this.assignments = assignments;
return this;
}

/**
* Adds or updates a member. This is useful when the updated member is
* not yet materialized in memory.
*
* @param memberId The member id.
* @param member The member to add or update.
* @return This object.
*/
public TargetAssignmentBuilder addOrUpdateMember(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm having a bit of trouble understanding how this will be used. i would think when we build a target assignment, we would already have all members updated/removed and snapshot the members state. this implies the state changes while we build the target assignment right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can actually see how it is used here. In short, when a heartbeat request is processed, we may have to recompute the target assignment before we persist the subscription changes because the records are applied to the state and persisted after the request is handled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it. it is because we have an existing group so we build the base target assignment from that existing group and if the member has changed we update the builder. and we can't update the group first in memory because we apply the records after the request is handled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.

String memberId,
ConsumerGroupMember member
) {
this.updatedMembers.put(memberId, member);
return this;
}

/**
* Removes a member. This is useful when the removed member
* is not yet materialized in memory.
*
* @param memberId The member id.
* @return This object.
*/
public TargetAssignmentBuilder removeMember(
String memberId
) {
return addOrUpdateMember(memberId, null);
}

/**
* Builds the new target assignment.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we should conform to use either assignments or assignment. i think assignment makes sense; it's inferred that the assignment covers all members and the object is called Assignment/TargetAssignment.

on the other hand, Assignment class is specific to a member so maybe assignments is more appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be even better to use target assignment in this class as it is means the target assignment "for the group".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm wondering if we should use rename Assignment to MemberAssignment to avoid confusion

Copy link
Contributor Author

@dajac dajac May 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was thinking about this as well but I did not because we already have a MemberAssignment in the assignor package. That causes issues when importing both of them. Overall, I think that Assignment is fine because it is often used in a Map<String, Assignment> so it is clear that it is per member.

*
* @return A TargetAssignmentResult which contains the records to update
* the existing target assignment.
* @throws PartitionAssignorException if the assignment can not be computed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: cannot

*/
public TargetAssignmentResult build() throws PartitionAssignorException {
Map<String, AssignmentMemberSpec> memberSpecs = new HashMap<>();

// Prepare the member spec for all members.
members.forEach((memberId, member) -> memberSpecs.put(memberId, createAssignmentMemberSpec(
member,
assignments.getOrDefault(memberId, Assignment.EMPTY),
subscriptionMetadata
)));

// Update the member spec if updated or deleted members.
updatedMembers.forEach((memberId, updatedMemberOrNull) -> {
if (updatedMemberOrNull == null) {
memberSpecs.remove(memberId);
} else {
memberSpecs.put(memberId, createAssignmentMemberSpec(
updatedMemberOrNull,
assignments.getOrDefault(memberId, Assignment.EMPTY),
subscriptionMetadata
));
}
});

// Prepare the topic metadata.
Map<Uuid, AssignmentTopicMetadata> topics = new HashMap<>();
subscriptionMetadata.forEach((topicName, topicMetadata) ->
topics.put(topicMetadata.id(), new AssignmentTopicMetadata(topicMetadata.numPartitions()))
);

// Compute the assignment.
GroupAssignment newGroupAssignment = assignor.assign(new AssignmentSpec(
Collections.unmodifiableMap(memberSpecs),
Collections.unmodifiableMap(topics)
));

// Compute delta from previous to new assignment and create the
// relevant records.
List<Record> records = new ArrayList<>();
Map<String, Assignment> newTargetAssignment = new HashMap<>();

memberSpecs.keySet().forEach(memberId -> {
Assignment oldMemberAssignment = assignments.get(memberId);
Assignment newMemberAssignment = newMemberAssignment(newGroupAssignment, memberId);

newTargetAssignment.put(memberId, newMemberAssignment);

if (oldMemberAssignment == null) {
// If the member had no assignment, we always create a record for it.
records.add(newTargetAssignmentRecord(
groupId,
memberId,
newMemberAssignment.partitions()
));
} else {
// If the member had an assignment, we only create a record if the
// new assignment is different.
if (!newMemberAssignment.equals(oldMemberAssignment)) {
records.add(newTargetAssignmentRecord(
groupId,
memberId,
newMemberAssignment.partitions()
));
}
}
});

// Bump the assignment epoch.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to confirm, the epoch will already be bumped when creating the Builder object right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is correct. We need to bump it in order to match the group epoch.

records.add(newTargetAssignmentEpochRecord(groupId, groupEpoch));

return new TargetAssignmentResult(records, newTargetAssignment);
}

private Assignment newMemberAssignment(
GroupAssignment newGroupAssignment,
String memberId
) {
MemberAssignment newMemberAssignment = newGroupAssignment.members().get(memberId);
if (newMemberAssignment != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when is the assignment null? When we are removing the group?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a defensive measure to protect us from the case where the assignor would not return an assignment for a member.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok -- so we don't typically expect this, but it isn't bad enough to throw an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it is much better to handle this case.

return new Assignment(newMemberAssignment.targetPartitions());
} else {
return Assignment.EMPTY;
}
}

public static AssignmentMemberSpec createAssignmentMemberSpec(
ConsumerGroupMember member,
Assignment targetAssignment,
Map<String, TopicMetadata> subscriptionMetadata
) {
Set<Uuid> subscribedTopics = new HashSet<>();
member.subscribedTopicNames().forEach(topicName -> {
TopicMetadata topicMetadata = subscriptionMetadata.get(topicName);
if (topicMetadata != null) {
subscribedTopics.add(topicMetadata.id());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To confirm -- we always expect to have an ID if the metadata is present.
What about the case where the metadata is not present? We just don't subscribe to the topic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. The id should be there because the new group coordinator will only work with kraft. If the metadata is not present, we just don't subscribe to the topic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we expect the metadata to be missing if we have the name in subscribedTopicNames? Do we expect to eventually get the metadata?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are two different things. As the consumer subscribe to a name, it could be that the topic does not exist yet for instance. In this case, we just ignore it and revise when the topic is created (if it ever is).

}
});

return new AssignmentMemberSpec(
Optional.ofNullable(member.instanceId()),
Optional.ofNullable(member.rackId()),
subscribedTopics,
targetAssignment.partitions()
);
}
}