-
Notifications
You must be signed in to change notification settings - Fork 14.3k
MINOR: Simplify PartitionStates update logic by removing unnecessary grouping #19442
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
base: trunk
Are you sure you want to change the base?
MINOR: Simplify PartitionStates update logic by removing unnecessary grouping #19442
Conversation
map.put(tp, state); | ||
} | ||
} | ||
map.putAll(partitionToState); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PartitionStates#update(Map<TopicPartition, S>)
is only used in PartitionStates#set(Map<TopicPartition, S>)
. Do you think that it's more clear to move the change to set
function directly? For example:
public void set(Map<TopicPartition, S> partitionToState) {
map.clear();
map.putAll(partitionToState);
updateSize();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion. I've updated the code to move the logic directly into the set
method as recommended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this patch, I have a question, Will this change the order in the map? Will it indirectly affect anything?
I’ve reviewed all call sites of Specifically:
In the original The new implementation with Given that the input maps are all unordered, this change does not affect observable behavior. |
Thanks @zt52875287 for explaination. |
This patch simplifies the implementation of PartitionStates#update by removing the unnecessary grouping of TopicPartitions by topic. The original logic constructed an intermediate map by topic name, but ultimately only inserted each (TopicPartition, state) pair into the internal LinkedHashMap. This grouping had no effect on the final result.
The updated implementation directly iterates over the input map, improving clarity and reducing overhead.
Additionally, the unit test has been updated to remove reliance on a specific iteration order of partition states. Since both PartitionStates#set and its internal update method accept a Map as input, and standard Java Maps do not guarantee any ordering beyond insertion order (which may not be preserved across implementations), tests should not assume a fixed output order unless explicitly defined by the API.