adixitconfluent commented on code in PR #19148:
URL: https://github.com/apache/kafka/pull/19148#discussion_r1990559413
##########
server/src/main/java/org/apache/kafka/server/share/fetch/PartitionRotateStrategy.java:
##########
@@ -80,20 +81,8 @@ static LinkedHashMap<TopicIdPartition, Integer>
rotateRoundRobin(
return topicIdPartitions;
}
- // TODO: Once the partition max bytes is removed then the partition
will be a linked list and rotation
- // will be a simple operation. Else consider using
ImplicitLinkedHashCollection.
- LinkedHashMap<TopicIdPartition, Integer> suffixPartitions = new
LinkedHashMap<>(rotateAt);
- LinkedHashMap<TopicIdPartition, Integer> rotatedPartitions = new
LinkedHashMap<>(topicIdPartitions.size());
- int i = 0;
- for (Map.Entry<TopicIdPartition, Integer> entry :
topicIdPartitions.entrySet()) {
- if (i < rotateAt) {
- suffixPartitions.put(entry.getKey(), entry.getValue());
- } else {
- rotatedPartitions.put(entry.getKey(), entry.getValue());
- }
- i++;
- }
- rotatedPartitions.putAll(suffixPartitions);
+ List<TopicIdPartition> rotatedPartitions = new
ArrayList<>(topicIdPartitions);
+ Collections.rotate(rotatedPartitions, -1 * rotateAt);
Review Comment:
I have added the comment `We want the elements from the end of the list to
move left by the distance provided i.e. if the original list is [1,2,3], and we
want to rotate it by 1, we want the output as [2,3,1] and not [3,1,2]. Hence,
we need negation of distance here` in the code. Regarding why I multiplied -1
to `rotateAt` instead of -`rotateAt`, its because `-1 * rotateAt` looks cleaner
to me.
##########
server/src/main/java/org/apache/kafka/server/share/fetch/PartitionRotateStrategy.java:
##########
@@ -80,20 +81,8 @@ static LinkedHashMap<TopicIdPartition, Integer>
rotateRoundRobin(
return topicIdPartitions;
}
- // TODO: Once the partition max bytes is removed then the partition
will be a linked list and rotation
- // will be a simple operation. Else consider using
ImplicitLinkedHashCollection.
- LinkedHashMap<TopicIdPartition, Integer> suffixPartitions = new
LinkedHashMap<>(rotateAt);
- LinkedHashMap<TopicIdPartition, Integer> rotatedPartitions = new
LinkedHashMap<>(topicIdPartitions.size());
- int i = 0;
- for (Map.Entry<TopicIdPartition, Integer> entry :
topicIdPartitions.entrySet()) {
- if (i < rotateAt) {
- suffixPartitions.put(entry.getKey(), entry.getValue());
- } else {
- rotatedPartitions.put(entry.getKey(), entry.getValue());
- }
- i++;
- }
- rotatedPartitions.putAll(suffixPartitions);
+ List<TopicIdPartition> rotatedPartitions = new
ArrayList<>(topicIdPartitions);
+ Collections.rotate(rotatedPartitions, -1 * rotateAt);
Review Comment:
I have added the comment `We want the elements from the end of the list to
move left by the distance provided i.e. if the original list is [1,2,3], and we
want to rotate it by 1, we want the output as [2,3,1] and not [3,1,2]. Hence,
we need negation of distance here` in the code. Regarding why I multiplied -1
to `rotateAt` instead of -`rotateAt`, its because `-1 * rotateAt` looks cleaner
code to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
