Skip to content
Closed
Show file tree
Hide file tree
Changes from 45 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
1 change: 1 addition & 0 deletions checkstyle/import-control.xml
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
<allow pkg="org.apache.kafka.common.utils" />
<allow pkg="org.apache.kafka.common.errors" exact-match="true" />
<allow pkg="org.apache.kafka.common.memory" />
<allow pkg="org.apache.kafka.coordinator.group.common"/>

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.

I suppose that we can remove this, isn't it?

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.

yep correct we don't need it now that we don't have any classes in it


<subpackage name="common">
<allow class="org.apache.kafka.clients.consumer.ConsumerRecord" exact-match="true" />
Expand Down
2 changes: 2 additions & 0 deletions checkstyle/suppressions.xml
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,8 @@

<!-- 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="MethodLength"
files="(GroupMetadataManager|ConsumerGroupTest|GroupMetadataManagerTest).java"/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,8 @@ private boolean allSubscriptionsEqual(Set<String> allTopics,

Set<String> subscribedTopics = new HashSet<>();

// keep track of all previously owned partitions so we can invalidate them if invalid input is
// detected, eg two consumers somehow claiming the same partition in the same/current generation
// keep track of all previously owned partitions, so we can invalidate them if invalid input is
// detected, e.g. two consumers somehow claiming the same partition in the same/current generation
Map<TopicPartition, String> allPreviousPartitionsToOwner = new HashMap<>();

for (Map.Entry<String, Subscription> subscriptionEntry : subscriptions.entrySet()) {
Expand Down Expand Up @@ -361,9 +361,9 @@ private boolean hasCycles(Set<ConsumerPair> pairs) {
}
}

// for now we want to make sure there is no partition movements of the same topic between a pair of consumers.
// for now, we want to make sure there is no partition movements of the same topic between a pair of consumers.
// the odds of finding a cycle among more than two consumers seem to be very low (according to various randomized
// tests with the given sticky algorithm) that it should not worth the added complexity of handling those cases.
// tests with the given sticky algorithm) that it should not be worth the added complexity of handling those cases.
for (List<String> cycle: cycles)
if (cycle.size() == 3) // indicates a cycle of length 2
return true;
Expand Down Expand Up @@ -806,7 +806,7 @@ private int assignNewPartition(TopicPartition unassignedPartition, String consum
private void verifyUnfilledMembers() {

if (!unfilledMembersWithUnderMinQuotaPartitions.isEmpty()) {
// we expected all the remaining unfilled members have minQuota partitions and we're already at the expected number
// we expected all the remaining unfilled members have minQuota partitions, and we're already at the expected number
// of members with more than the minQuota partitions. Otherwise, there must be error here.
if (currentNumMembersWithOverMinQuotaPartitions != expectedNumMembersWithOverMinQuotaPartitions) {
log.error("Current number of members with more than the minQuota partitions: {}, is less than the expected number " +
Expand Down Expand Up @@ -896,7 +896,7 @@ private List<TopicPartition> getUnassignedPartitions(List<TopicPartition> sorted
* while preserving rack-alignment. This step is used only for rack-aware assignment.
* 4. Assigning the remaining unassigned partitions in a way that balances out the overall assignments of partitions to consumers.
* For rack-aware algorithm, these are partitions that could not be aligned on racks within the balancing constraints.
* 5. Further balancing out the resulting assignment by finding the partitions that can be reassigned
* 5. Further, balancing out the resulting assignment by finding the partitions that can be reassigned
* to another consumer towards an overall more balanced assignment. For rack-aware algorithm, attempt
* to retain rack alignment if possible.
*
Expand Down Expand Up @@ -1021,7 +1021,7 @@ private List<TopicPartition> assignOwnedPartitions() {
revocationRequired = true;
} else {
// otherwise, remove the topic partition from those that need to be assigned only if
// its current consumer is still subscribed to its topic (because it is already assigned
// its current consumer is still subscribed to its topic (because it is already assigned,
// and we would want to preserve that assignment as much as possible)
assignedPartitions.add(partition);
}
Expand Down Expand Up @@ -1075,7 +1075,7 @@ private List<TopicPartition> getUnassignedPartitions(List<TopicPartition> sorted
* update the prevAssignment with the partitions, consumer and generation in parameters
*
* @param partitions: The partitions to be updated the prevAssignment
* @param consumer: The consumer Id
* @param consumer: The consumerId
* @param prevAssignment: The assignment contains the assignment with the 2nd largest generation
* @param generation: The generation of this assignment (partitions)
*/
Expand Down Expand Up @@ -1321,7 +1321,7 @@ private void balance(Map<TopicPartition, ConsumerGenerationPair> prevAssignment,
fixedAssignments.put(consumer, currentAssignment.remove(consumer));
}

// create a deep copy of the current assignment so we can revert to it if we do not get a more balanced assignment later
// create a deep copy of the current assignment, so we can revert to it if we do not get a more balanced assignment later
Map<String, List<TopicPartition>> preBalanceAssignment = deepCopy(currentAssignment);
Map<TopicPartition, String> preBalancePartitionConsumers = new HashMap<>(currentPartitionConsumer);

Expand All @@ -1332,7 +1332,7 @@ private void balance(Map<TopicPartition, ConsumerGenerationPair> prevAssignment,

boolean reassignmentPerformed = performReassignments(sortedAllPartitions, prevAssignment);

// if we are not preserving existing assignments and we have made changes to the current assignment
// if we are not preserving existing assignments, and we have made changes to the current assignment
// make sure we are getting a more balanced assignment; otherwise, revert to previous assignment
if (!initializing && reassignmentPerformed && getBalanceScore(currentAssignment) >= getBalanceScore(preBalanceAssignment)) {
deepCopy(preBalanceAssignment, currentAssignment);
Expand All @@ -1358,7 +1358,7 @@ private boolean performReassignments(List<TopicPartition> reassignablePartitions
// repeat reassignment until no partition can be moved to improve the balance
do {
modified = false;
// reassign all reassignable partitions (starting from the partition with least potential consumers and if needed)
// reassign all reassignable partitions (starting from the partition with the least potential consumers and if needed)
// until the full list is processed or a balance is achieved
Iterator<TopicPartition> partitionIterator = reassignablePartitions.iterator();
while (partitionIterator.hasNext() && !isBalanced()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,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 @@ -609,7 +610,7 @@ private CoordinatorResult<ConsumerGroupHeartbeatResponseData, Record> consumerGr

try {
TargetAssignmentBuilder.TargetAssignmentResult assignmentResult =
new TargetAssignmentBuilder(groupId, groupEpoch, assignors.get(preferredServerAssignor))
new TargetAssignmentBuilder(groupId, groupEpoch,assignors.get(preferredServerAssignor))
.withMembers(group.members())
.withSubscriptionMetadata(subscriptionMetadata)
.withTargetAssignment(group.targetAssignment())
Expand Down Expand Up @@ -700,7 +701,8 @@ private CoordinatorResult<ConsumerGroupHeartbeatResponseData, Record> consumerGr
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 @@ -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 @@ -30,19 +28,11 @@ public class AssignmentSpec {
*/
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;
}

/**
Expand All @@ -52,33 +42,22 @@ 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;
if (!(o instanceof AssignmentSpec)) return false;
Comment thread
rreddy-22 marked this conversation as resolved.
Outdated
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 Objects.hash(members);
Comment thread
rreddy-22 marked this conversation as resolved.
Outdated
}

@Override
public String toString() {
return "AssignmentSpec(members=" + members +
", topics=" + topics +
')';
Comment thread
rreddy-22 marked this conversation as resolved.
Outdated
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
* 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 assignment 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 AssignmentTopicDescriber {

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.

nit: I wonder if SubscribedTopicDescriber would be better based on the javadoc.

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.

Yeah I named it this way cause I was just wondering if it'd be more uniform with assignmentSpec but I'll change it cause I agree


/**
* Returns a set of subscribed topicIds.
*
* @return Set of topicIds corresponding to the subscribed topics.
*/
Set<Uuid> subscribedTopicIds();

/**
* Number of partitions for the given topicId.

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.

nit: topicIds -> topic id.

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.

It says topicId singular already, did we want a space between topic and 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 0;
*/
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.

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.

nit: Let's use partition id or index instead of number.

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.

partition number is used a lot throughout the kafka code and I thought it's easier to understand than Id even though they're interchangeable.

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.

partition number is used a lot throughout the kafka code

Is it? I don't remember seeing it. I usually see partition index or id.

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.

yeah haha I'll make it partition Id

* @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 @@ -16,44 +16,83 @@
*/
package org.apache.kafka.coordinator.group.assignor;

/**
* Metadata of a topic.
*/
public class AssignmentTopicMetadata {
import org.apache.kafka.common.Uuid;

import java.util.Collections;
import java.util.Map;
import java.util.Objects;
import java.util.Set;

public class AssignmentTopicMetadata implements AssignmentTopicDescriber {
Comment thread
rreddy-22 marked this conversation as resolved.
Outdated

Map<Uuid, PartitionMetadata> TopicPartitionMetadata;
Comment thread
rreddy-22 marked this conversation as resolved.
Outdated

public AssignmentTopicMetadata(Map<Uuid, PartitionMetadata> TopicPartitionMetadata) {
this.TopicPartitionMetadata = TopicPartitionMetadata;
}

/**
* Returns a set of subscribed topicIds.
*
* @return Set of topicIds corresponding to the subscribed topics.
*/
@Override
public Set<Uuid> subscribedTopicIds() {
return TopicPartitionMetadata.keySet();
}

/**
* The number of partitions.
* Number of partitions for the given topicId.
*
* @param topicId Uuid corresponding to the topic.
* @return The number of partitions corresponding to the given topicId.
* If the topicId doesn't exist return 0;
*/
private final int numPartitions;
@Override
public int numPartitions(Uuid topicId) {
PartitionMetadata partitionMetadata = TopicPartitionMetadata.get(topicId);
if (partitionMetadata == null) {
return 0;
}

public AssignmentTopicMetadata(
int numPartitions
) {
this.numPartitions = numPartitions;
return partitionMetadata.numPartitions();
}

/**
* @return The number of partitions present for the topic.
* 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;
*/
public int numPartitions() {
return numPartitions;
@Override
public Set<String> racksForPartition(Uuid topicId, int partition) {
PartitionMetadata partitionMetadata = TopicPartitionMetadata.get(topicId);
if (partitionMetadata == null) {
return Collections.emptySet();
}

return partitionMetadata.racks(partition);
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
if (!(o instanceof AssignmentTopicMetadata)) return false;
Comment thread
rreddy-22 marked this conversation as resolved.
Outdated
AssignmentTopicMetadata that = (AssignmentTopicMetadata) o;
return numPartitions == that.numPartitions;
return TopicPartitionMetadata.equals(that.TopicPartitionMetadata);
}

@Override
public int hashCode() {
return numPartitions;
return Objects.hash(TopicPartitionMetadata);
Comment thread
rreddy-22 marked this conversation as resolved.
Outdated
}

@Override
public String toString() {
return "AssignmentTopicMetadata(numPartitions=" + numPartitions + ')';
return "AssignmentTopicMetadata{" +
Comment thread
rreddy-22 marked this conversation as resolved.
Outdated
"TopicPartitionMetadata=" + TopicPartitionMetadata +
'}';
}
}
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 assignmentTopicDescriber The topic and cluster metadata describer {@link AssignmentTopicDescriber}.
* @param assignmentSpec The member assignment spec.
* @return The new assignment for the group.
*/
GroupAssignment assign(AssignmentSpec assignmentSpec) throws PartitionAssignorException;
GroupAssignment assign(AssignmentTopicDescriber assignmentTopicDescriber, AssignmentSpec assignmentSpec) throws PartitionAssignorException;
Comment thread
rreddy-22 marked this conversation as resolved.
Outdated
}
Loading