Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
e8d3607
Server Side Sticky Range Assignor full implementation
rreddy-22 Mar 23, 2023
3df3460
removed reduce partitions case
rreddy-22 Mar 28, 2023
7410529
removed reduce partitions case
rreddy-22 Mar 28, 2023
3b6e65e
Addressed PR comments
rreddy-22 Mar 29, 2023
4b321aa
Addressed PR comments
rreddy-22 Apr 3, 2023
b4041a7
Merge branch 'apache:trunk' into rreddy-22/KAFKA-14514
rreddy-22 Apr 6, 2023
7352cbb
minor
rreddy-22 Apr 6, 2023
57b94ca
Made subscribed topics a collection, removed * import exception
rreddy-22 Apr 6, 2023
5065109
Interface changes for assignment map and subscribed topics, added new…
rreddy-22 Apr 6, 2023
100a04e
Added toString method and hash
rreddy-22 Apr 7, 2023
73c7fdc
Made all attributes private and added getter methods, changed names a…
rreddy-22 Apr 11, 2023
68d53b3
Removed topicIdToPartition class, changed attribute names and added j…
rreddy-22 Apr 12, 2023
48a9822
Server Side Sticky Range Assignor full implementation
rreddy-22 Mar 23, 2023
dcb8198
removed reduce partitions case
rreddy-22 Mar 28, 2023
9f3a423
removed reduce partitions case
rreddy-22 Mar 28, 2023
a925b02
Addressed PR comments
rreddy-22 Mar 29, 2023
7d16269
Addressed PR comments
rreddy-22 Apr 3, 2023
7369db3
minor
rreddy-22 Apr 6, 2023
918d262
Made subscribed topics a collection, removed * import exception
rreddy-22 Apr 6, 2023
288fa1f
Addressed some PR comments
rreddy-22 Apr 12, 2023
a1bdb58
Removed putList, putSet, changed generic pair to remainingAssignments…
rreddy-22 Apr 13, 2023
0737c8d
Merge remote-tracking branch 'origin/rreddy-22/KAFKA-14514' into rred…
rreddy-22 Apr 17, 2023
3d92645
Interface changes incorporated, added stickiness test and non-existen…
rreddy-22 Apr 17, 2023
309cc61
Merge branch 'trunk' into rreddy-22/KAFKA-14514
rreddy-22 Apr 17, 2023
7fd7794
First draft optimised uniform assignor code
rreddy-22 Apr 5, 2023
dffad67
Fixed checkstyle and added test file
rreddy-22 Apr 6, 2023
ef81486
import changes
rreddy-22 Apr 7, 2023
0cd618a
Separated uniform and general builder in different files, added more …
rreddy-22 Apr 11, 2023
d33bbfa
Added stickiness test
rreddy-22 Apr 12, 2023
2d50c27
Renamed topicIdToPartition class
rreddy-22 Apr 13, 2023
e56ded2
Interface changes incorporated
rreddy-22 Apr 17, 2023
28ab298
consumers -> members
rreddy-22 Apr 19, 2023
5550e49
fixed formatting stuff
rreddy-22 Apr 20, 2023
feb12ec
grammar and code comments changes
rreddy-22 Apr 25, 2023
cc88822
small changes
rreddy-22 May 10, 2023
1c2475b
Squashed commit of the following:
rreddy-22 May 12, 2023
e1a0325
small changes
rreddy-22 May 16, 2023
931bcf7
test changes and code changes
rreddy-22 May 31, 2023
e9ebc4f
Refactored code to reduce format conversion time
rreddy-22 Jun 29, 2023
9815d6d
Abstract Partition Assignor
rreddy-22 Jul 7, 2023
ce540f9
Merge remote-tracking branch 'upstream/trunk' into rreddy-22/KAFKA-14…
rreddy-22 Jul 7, 2023
478488e
Added rack interface changes, added a new TopicAndClusterMetadata cla…
rreddy-22 Jul 11, 2023
c50f3bd
Removed extra lines
rreddy-22 Jul 11, 2023
f68471a
Small edits
rreddy-22 Jul 13, 2023
579bb94
New interfaces and classes added to facilitate passing rack informati…
rreddy-22 Jul 19, 2023
4511683
Changes based on PR comments
rreddy-22 Jul 21, 2023
faafed2
Merge branch 'trunk' into rreddy-22/Rack-Assignor-Interface-Changes
rreddy-22 Jul 21, 2023
22f2df0
minor changes
rreddy-22 Jul 24, 2023
6650ea5
Merge remote-tracking branch 'origin/rreddy-22/Rack-Assignor-Interfac…
rreddy-22 Jul 24, 2023
061dac7
Moved SubscribedTopicMetadata to the consumer package
rreddy-22 Jul 24, 2023
57b9a62
Merge branch 'apache:trunk' into rreddy-22/Rack-Assignor-Interface-Ch…
rreddy-22 Jul 24, 2023
a30a534
minor
rreddy-22 Jul 24, 2023
6fe72df
minor
rreddy-22 Jul 24, 2023
c0b464c
reverted grammar changes
rreddy-22 Jul 24, 2023
a39e76f
reverted reviewers.py changes
rreddy-22 Jul 24, 2023
2d40125
minor
rreddy-22 Jul 24, 2023
81f1ccd
Merge branch 'apache:trunk' into rreddy-22/Rack-Assignor-Interface-Ch…
rreddy-22 Jul 24, 2023
1c955b5
Merge branch 'rreddy-22/Rack-Assignor-Interface-Changes' of github.co…
rreddy-22 Jul 24, 2023
5a93532
Minor
rreddy-22 Jul 25, 2023
71f8488
Deleted PartitionMetadata and added TopicMetadata to the SubscribedTo…
rreddy-22 Jul 25, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions checkstyle/suppressions.xml
Original file line number Diff line number Diff line change
Expand Up @@ -321,13 +321,13 @@

<!-- group coordinator -->
<suppress checks="CyclomaticComplexity"
files="(ConsumerGroupMember).java"/>
<suppress checks="ParameterNumber"
Comment thread
rreddy-22 marked this conversation as resolved.
files="(ConsumerGroupMember|GroupMetadataManager).java"/>
<suppress checks="(NPathComplexity|MethodLength)"
files="(GroupMetadataManager|ConsumerGroupTest|GroupMetadataManagerTest).java"/>
<suppress checks="ClassFanOutComplexity"
files="(GroupMetadataManager|GroupMetadataManagerTest).java"/>
<suppress checks="ParameterNumber"
files="(ConsumerGroupMember|GroupMetadataManager).java"/>
<suppress checks="ClassDataAbstractionCouplingCheck"
files="(RecordHelpersTest|GroupMetadataManagerTest).java"/>
<suppress checks="JavaNCSS"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -777,7 +777,8 @@ private CoordinatorResult<ConsumerGroupHeartbeatResponseData, Record> consumerGr
subscriptionMetadata = group.computeSubscriptionMetadata(
member,
updatedMember,
metadataImage.topics()
metadataImage.topics(),
metadataImage.cluster()
);

if (!subscriptionMetadata.equals(group.subscriptionMetadata())) {
Expand Down Expand Up @@ -927,7 +928,8 @@ private List<Record> consumerGroupFenceMember(
Map<String, TopicMetadata> subscriptionMetadata = group.computeSubscriptionMetadata(
member,
null,
metadataImage.topics()
metadataImage.topics(),
metadataImage.cluster()
);

if (!subscriptionMetadata.equals(group.subscriptionMetadata())) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,13 +131,21 @@ public static Record newGroupSubscriptionMetadataRecord(
Map<String, TopicMetadata> newSubscriptionMetadata
) {
ConsumerGroupPartitionMetadataValue value = new ConsumerGroupPartitionMetadataValue();
newSubscriptionMetadata.forEach((topicName, topicMetadata) ->
newSubscriptionMetadata.forEach((topicName, topicMetadata) -> {
List<ConsumerGroupPartitionMetadataValue.PartitionMetadata> partitionMetadata = new ArrayList<>();
topicMetadata.partitionRacks().forEach((partition, racks) ->
partitionMetadata.add(new ConsumerGroupPartitionMetadataValue.PartitionMetadata()
.setPartition(partition)
.setRacks(new ArrayList<>(racks))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we set racks all the time or only when the set is non-empty?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I changed the code now, if no racks are available then an empty map is stored in the topicMetadata and therefore in the subscription metadata. Moreover, an empty list will be stored in the record to save memory

)
);
value.topics().add(new ConsumerGroupPartitionMetadataValue.TopicMetadata()
.setTopicId(topicMetadata.id())
.setTopicName(topicMetadata.name())
.setNumPartitions(topicMetadata.numPartitions())
)
);
.setPartitionMetadata(partitionMetadata)
);
});

return new Record(
new ApiMessageAndVersion(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@
*/
package org.apache.kafka.coordinator.group.assignor;

import org.apache.kafka.common.Uuid;

import java.util.Map;
import java.util.Objects;

Expand All @@ -26,59 +24,39 @@
*/
public class AssignmentSpec {
/**
* The members keyed by member id.
* The members keyed by member Id.
*/
private final Map<String, AssignmentMemberSpec> members;

/**
* The topics' metadata keyed by topic id.
*/
private final Map<Uuid, AssignmentTopicMetadata> topics;

public AssignmentSpec(
Map<String, AssignmentMemberSpec> members,
Map<Uuid, AssignmentTopicMetadata> topics
Map<String, AssignmentMemberSpec> members
) {
Objects.requireNonNull(members);
Objects.requireNonNull(topics);
this.members = members;
this.topics = topics;
}

/**
* @return Member metadata keyed by member Ids.
* @return Member metadata keyed by member Id.
*/
public Map<String, AssignmentMemberSpec> members() {
return members;
}

/**
* @return Topic metadata keyed by topic Ids.
*/
public Map<Uuid, AssignmentTopicMetadata> topics() {
return topics;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
AssignmentSpec that = (AssignmentSpec) o;
if (!members.equals(that.members)) return false;
return topics.equals(that.topics);
return members.equals(that.members);
}

@Override
public int hashCode() {
int result = members.hashCode();
result = 31 * result + topics.hashCode();
return result;
return members.hashCode();
}

@Override
public String toString() {
return "AssignmentSpec(members=" + members +
", topics=" + topics +
')';
return "AssignmentSpec(members=" + members + ')';
}
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,9 @@ public interface PartitionAssignor {
* Perform the group assignment given the current members and
* topic metadata.
*
* @param assignmentSpec The assignment spec.
* @param assignmentSpec The member assignment spec.
* @param subscribedTopicDescriber The topic and cluster metadata describer {@link SubscribedTopicDescriber}.
* @return The new assignment for the group.
*/
GroupAssignment assign(AssignmentSpec assignmentSpec) throws PartitionAssignorException;
GroupAssignment assign(AssignmentSpec assignmentSpec, SubscribedTopicDescriber subscribedTopicDescriber) throws PartitionAssignorException;
}
Original file line number Diff line number Diff line change
Expand Up @@ -78,23 +78,19 @@ public MemberWithRemainingAssignments(String memberId, int remaining) {
/**
* @return Map of topic ids to a list of members subscribed to them.
*/
private Map<Uuid, List<String>> membersPerTopic(final AssignmentSpec assignmentSpec) {
private Map<Uuid, List<String>> membersPerTopic(final AssignmentSpec assignmentSpec, final SubscribedTopicDescriber subscribedTopicDescriber) {
Map<Uuid, List<String>> membersPerTopic = new HashMap<>();
Map<String, AssignmentMemberSpec> membersData = assignmentSpec.members();

membersData.forEach((memberId, memberMetadata) -> {
Collection<Uuid> topics = memberMetadata.subscribedTopicIds();
for (Uuid topicId : topics) {
// Only topics that are present in both the subscribed topics list and the topic metadata should be
// considered for assignment.
if (assignmentSpec.topics().containsKey(topicId)) {
membersPerTopic
.computeIfAbsent(topicId, k -> new ArrayList<>())
.add(memberId);
} else {
throw new PartitionAssignorException("Member " + memberId + " subscribed to topic " +
topicId + " which doesn't exist in the topic metadata");
if (subscribedTopicDescriber.numPartitions(topicId) == -1) {
throw new PartitionAssignorException("Member is subscribed to a non-existent topic");
}
membersPerTopic
.computeIfAbsent(topicId, k -> new ArrayList<>())
.add(memberId);
}
});

Expand All @@ -118,14 +114,14 @@ private Map<Uuid, List<String>> membersPerTopic(final AssignmentSpec assignmentS
* </ol>
*/
@Override
public GroupAssignment assign(final AssignmentSpec assignmentSpec) throws PartitionAssignorException {
public GroupAssignment assign(final AssignmentSpec assignmentSpec, final SubscribedTopicDescriber subscribedTopicDescriber) throws PartitionAssignorException {
Map<String, MemberAssignment> newAssignment = new HashMap<>();

// Step 1
Map<Uuid, List<String>> membersPerTopic = membersPerTopic(assignmentSpec);
Map<Uuid, List<String>> membersPerTopic = membersPerTopic(assignmentSpec, subscribedTopicDescriber);

membersPerTopic.forEach((topicId, membersForTopic) -> {
int numPartitionsForTopic = assignmentSpec.topics().get(topicId).numPartitions();
int numPartitionsForTopic = subscribedTopicDescriber.numPartitions(topicId);
int minRequiredQuota = numPartitionsForTopic / membersForTopic.size();
// Each member can get only ONE extra partition per topic after receiving the minimum quota.
int numMembersWithExtraPartition = numPartitionsForTopic % membersForTopic.size();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
* 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.assignor;

import org.apache.kafka.common.Uuid;
import org.apache.kafka.common.annotation.InterfaceStability;

import java.util.Set;

/**
* The subscribed topic describer is used by the {@link PartitionAssignor}
* to obtain topic and partition metadata of subscribed topics.
*
* The interface is kept in an internal module until KIP-848 is fully
* implemented and ready to be released.
*/
@InterfaceStability.Unstable
public interface SubscribedTopicDescriber {
/**
* Number of partitions for the given topic Id.
*
* @param topicId Uuid corresponding to the topic.
* @return The number of partitions corresponding to the given topicId.
* If the topicId doesn't exist return -1;
*/
int numPartitions(Uuid topicId);

/**
* Returns all the racks associated with the replicas for the given partition.
*
* @param topicId Uuid corresponding to the partition's topic.
* @param partition Partition number within topic.
* @return The set of racks corresponding to the replicas of the topics partition.
* If the topicId doesn't exist return an empty set;
*/
Set<String> racksForPartition(Uuid topicId, int partition);
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.apache.kafka.common.Uuid;
import org.apache.kafka.common.errors.UnknownMemberIdException;
import org.apache.kafka.coordinator.group.Group;
import org.apache.kafka.image.ClusterImage;
import org.apache.kafka.image.TopicImage;
import org.apache.kafka.image.TopicsImage;
import org.apache.kafka.timeline.SnapshotRegistry;
Expand All @@ -28,6 +29,7 @@

import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
Expand Down Expand Up @@ -434,22 +436,39 @@ public void setSubscriptionMetadata(
public Map<String, TopicMetadata> computeSubscriptionMetadata(
ConsumerGroupMember oldMember,
ConsumerGroupMember newMember,
TopicsImage topicsImage
TopicsImage topicsImage,
ClusterImage clusterImage
Comment on lines +439 to +440

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it make sense to directly pass MetadataImage?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since metadataImage has a lot more images that we don't need and passing the specific arguments makes it more readable, maintainable, and testable I feel like we can keep it as two separate arguments.

) {
// Copy and update the current subscriptions.
Map<String, Integer> subscribedTopicNames = new HashMap<>(this.subscribedTopicNames);
maybeUpdateSubscribedTopicNames(subscribedTopicNames, oldMember, newMember);

// Create the topic metadata for each subscribed topic.
Map<String, TopicMetadata> newSubscriptionMetadata = new HashMap<>(subscribedTopicNames.size());

subscribedTopicNames.forEach((topicName, count) -> {
TopicImage topicImage = topicsImage.getTopic(topicName);
if (topicImage != null) {
Map<Integer, Set<String>> partitionRacks = new HashMap<>();

topicImage.partitions().forEach((partition, partitionRegistration) -> {
Set<String> racks = new HashSet<>();
for (int replica : partitionRegistration.replicas) {
Optional<String> rackOptional = clusterImage.broker(replica).rack();
// Only add rack if it is available for the broker/replica.
rackOptional.ifPresent(racks::add);
}
// If no racks are available for any replica of this partition, store an empty map.
if (!racks.isEmpty())
partitionRacks.put(partition, racks);
});

newSubscriptionMetadata.put(topicName, new TopicMetadata(
topicImage.id(),
topicImage.name(),
topicImage.partitions().size()
));
topicImage.partitions().size(),
partitionRacks)
);
}
});

Expand Down
Loading