[ 
https://issues.apache.org/jira/browse/KAFKA-347?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13713045#comment-13713045
 ] 

Joel Koshy commented on KAFKA-347:
----------------------------------

Thank you for the patch. Couple of comments, all very minor:

AddPartitionsCommand:
- IMO it is more intuitive for the option to be: "total partitions desired"
  as opposed to "num partitions to add"
- It is a bit odd that we can allow some partitions with a different
  replication factor from what's already there. I don't see any issues with
  it though. I just think it's a bit odd. One potential issue is if
  producers explicitly want to set acks to say 3 when there are some
  partitions with a replication factor of 2 and some with 3 (However,
  producers really should be using -1 in which case it would be fine).
- I think the command can currently allow an unintentional reassignment of
  replicas since the persistent path is always updated. (or no?) I think
  this can be easily checked for and avoided.
- Apart from start partition id I think getManualReplicaAssignment is
  identical to CreateTopicCommand's - maybe that code can be moved into
  AdminUtils?

KafkaController:
- nitpick: ZkUtils.getAllTopics(zkClient).foreach(p =>
  partitionStateMachine.registerPartitionChangeListener(p)) (can you change
  p to t :) - p really looks like a partition but it is a topic )

AdminUtils:
- the //"for testing only" comment is now misplaced.
- This code is pre-existing, but would prefer changing secondReplicaShift to
  nextReplicaShift.

- Any reason why AddPartitionsTest should not be within AdminTest?
- Finally, can you rebase? Sorry for not getting to this patch sooner :(


                
> change number of partitions of a topic online
> ---------------------------------------------
>
>                 Key: KAFKA-347
>                 URL: https://issues.apache.org/jira/browse/KAFKA-347
>             Project: Kafka
>          Issue Type: Improvement
>          Components: core
>    Affects Versions: 0.8
>            Reporter: Jun Rao
>            Assignee: Sriram Subramanian
>              Labels: features
>             Fix For: 0.8.1
>
>         Attachments: kafka-347.patch
>
>
> We will need an admin tool to change the number of partitions of a topic 
> online.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Reply via email to