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
