Hi Tom, What do you think of moving `increasePartitionsCount` (or `increaseNumPartitions`) to a separate KIP? That is simple enough that we could potentially include it in 1.0.0. I'd be happy to review it. ReassignPartitions is more complex and we can probably aim to include that in the January release. What do you think?
Ismael On Wed, Sep 6, 2017 at 11:42 PM, Colin McCabe <cmcc...@apache.org> wrote: > On Wed, Sep 6, 2017, at 00:20, Tom Bentley wrote: > > Hi Ted and Colin, > > > > Thanks for the comments. > > > > It seems you're both happier with reassign rather than assign, so I'm > > happy > > to stick with that. > > > > > > On 5 September 2017 at 18:46, Colin McCabe <cmcc...@apache.org> wrote: > > > > > ... > > > > > > > Do we expect that reducing the number of partitions will ever be > > > supported by this API? It seems like decreasing would require a > > > different API-- one which supported data movement, had a "check status > > > of this operation" feature, etc. etc. If this API is only ever going > to > > > be used to increase the number of partitions, I think we should just > > > call it "increasePartitionCount" to avoid confusion. > > > > > > > I thought a little about the decrease possibility (hence the static > > factory > > methods on PartitionCount), but not really in enough detail. I suppose a > > decrease process could look like this: > > > > 1. Producers start partitioning based on the decreased partition count. > > 2. Consumers continue to consume from all partitions. > > 3. At some point all the records in the old partitions have expired and > > they can be deleted. > > > > This wouldn't work for compacted topics. Of course a more aggressive > > strategy is also possible where we forcibly move data from the partitions > > to be deleted. > > > > Anyway, in either case the process would be long running, whereas the > > increase case is fast, so the semantics are quite different. So I agree, > > lets rename the method to make clear that it's only for increasing the > > partition count. > > > > > > > > > Outstanding questions: > > > > > > > > 1. Is the proposed alterInterReplicaThrottle() API really better than > > > > changing the throttle via alterConfigs()? > > > > > > That's a good point. I would argue that we should just use > alterConfigs > > > to set the broker configuration, rather than having a special RPC just > > > for this. > > > > > > > Yes, I'm minded to agree. > > > > The reason I originally thought a special API might be better was if > > people > > felt that the DynamicConfig mechanism (which seems to exist only to > > support > > these throttles) was an implementation detail of the throttles. But I now > > realise that they're visible via kafka-topics.sh, so they're effectively > > already a public API. > > > > > > > > > > ... > > > > Would it be a problem that > > > > triggering the reassignment required ClusterAction on the Cluster, > but > > > > throttling the assignment required Alter on the Topic? What if a > user had > > > > the former permission, but not the latter? > > > > > > We've been trying to reserve ClusterAction on Cluster for > > > broker-initiated operations. Alter on Cluster is the ACL for "root > > > stuff" and I would argue that it should be what we use here. > > > > > > For reconfiguring the broker, I think we should follow KIP-133 and use > > > AlterConfigs on the Broker resource. (Of course, if you just use the > > > existing alterConfigs call, you get this without any special effort.) > > > > > > > Yes, sorry, what I put in the email about authorisation wasn't what I put > > in the KIP (I revised the KIP after drafting the email and then forgot to > > update the email). > > > > Although KIP-133 proposes a Broker resource, I don't see one in the code > > and KIP-133 was supposedly delivered in 0.11. Can anyone fill in the > > story > > here? Is it simply because the functionality to update broker configs > > hasn't been implemented yet? > > Look in > ./clients/src/main/java/org/apache/kafka/common/config/ > ConfigResource.java, > for the BROKER resource. I bet you're looking at the Resource class > used for ACLs, which is a different class. > > > > > As currently proposed, both reassignPartitions() and > > alterInterBrokerThrottle() > > require Alter on the Cluster. If we used alterConfigs() to set the > > throttles then we create a situation where the triggering of the > > reassignment required Alter(Cluster), but the throttling required > > Alter(Broker), and the user might have the former but not the latter. I > > don't think this is likely to be a big deal in practice, but maybe others > > disagree? > > Alter:Cluster is essentially root, though. If you have Alter:Cluster > and you don't have AlterConfigs:Broker, you can just create a new ACL > giving it to yourself (creating and deleting ACLs is one of the powers > of Alter:Cluster) > > cheers, > Colin > > > > > > > > > > > > > 2. Is reassignPartitions() really the best name? I find the > distinction > > > > between reassigning and just assigning to be of limited value, and > > > > "reassign" is misleading when the APIs now used for changing the > > > > replication factor too. Since the API is asynchronous/long running, > it > > > > might be better to indicate that in the name some how. What about > > > > startPartitionAssignment()? > > > > > > Good idea -- I like the idea of using "start" or "initiate" to indicate > > > that this is kicking off a long-running operation. > > > > > > "reassign" seemed like a natural choice to me since this is changing an > > > existing assignment. It was assigned (when the topic it was created)-- > > > now it's being re-assigned. If you change it to just "assign" then it > > > feels confusing to me. A new user might ask if "assigning partitions" > > > is a step that they should take after creating a topic, similar to how > > > subscribing to topics is a step you take after creating a consumer. > > > > > > > Yeah, I find this new user argument persuasive, so I'm happy to stick > > with > > reassign. > > > > Thanks again for the feedback, > > > > Tom >
