[GitHub] [kafka] andymg3 commented on pull request #12892: KAFKA-14386: Change ReplicaPlacer place method to return a class instead of a list of list of integers

[GitBox <gi...@apache.org>]

andymg3 commented on PR #12892:
URL: https://github.com/apache/kafka/pull/12892#issuecomment-1333944951

   > Thanks for the changes @andymg3 .
   > 
   > Should we also improve the `PartitionRegistration` type to use a `PartitionAssignment` instead of `int[] replicas`?
   
   Thanks for the review @jsancio. As of now I dont see much benefit in changing that type. In `PartitionRegistration` we also have `[]int isr`, `int[] removingReplicas`, and `int[] addingReplicas`. I'm included to leave `replicas` as is for consistency - I think it reads a bit better when they're all the same type. They're often processed at the same time as well, so having to deal with both types may be a bit cumbersome. There are quite a few places in the codebase that use the `replicas` field and in practice if we were to use `PartitionAssignment`  all callers would mostly just call `PartitionAssignment.replicas()` anyways. So overall I don't see much benefit in introducing the type. 
   
   What do you think? 


-- 
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: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org