Hi Ismael, OK, KIP-195 has been factored out.
Regarding the dynamic configs, I personally still think we should have a > specific protocol API for that Can you explain a little more why? With regards to throttling, it would be > worth thinking about a way where the throttling configs can be > automatically removed without the user having to re-run the tool. > Isn't that just a matter of updating the topic configs for (leader|follower).replication.throttled.replicas at the same time we remove the reassignment znode? That leaves open the question about whether to reset the rates at the same time. But now I wonder what the broker configs "(leader|follower).replication.throttled.rate" are DynamicConfigs but the topic configs "(leader|follower).replication.throttled.replicas" are normal configs. Aren't the topic ones for the replicas just as dynamic as the broker ones for the rate? Thanks, Tom On 7 September 2017 at 17:24, Ismael Juma <ism...@juma.me.uk> wrote: > Hi Tom, > > It won't be used within Kafka, but it's a public API that can be used by > other projects. And the protocol can be used by non-Java clients. So, there > is still value in including it. > > Regarding the dynamic configs, I personally still think we should have a > specific protocol API for that. With regards to throttling, it would be > worth thinking about a way where the throttling configs can be > automatically removed without the user having to re-run the tool. So, yes, > maybe it should be a separate KIP as well. > > Not sure if we need it in the template, but you're welcome to mention any > dependencies when there are some. > > Thanks, > Ismael > > On Thu, Sep 7, 2017 at 5:15 PM, Tom Bentley <t.j.bent...@gmail.com> wrote: > > > Hi Ismael, > > > > It would be good to get at least some of this into 1.0.0. > > > > We could put the increasePartitions() work into another KIP, but it would > > be an unused AdminClient API in that release. The consumer of this API > will > > be the TopicsCommand when that get refactored to use the AdminClient. > > That's something Paolo Patierno is proposing to do but afaik not in time > > for 1.0.0. I don't think that's an issue, though, so I'll split out a > > separate KIP. > > > > FWIW, we could also split out the proposal to support describeConfigs() > and > > alterConfigs() for dynamic configs into a separate KIP too. But that's > also > > a decision we can defer until we're looking at the remainder of KIP-179. > > > > Aside: I wonder if it would be a good idea for the KIP template to have a > > "Depends on" field so people can more easily keep track of how multiple > > in-flight KIPs depend on one another? > > > > Cheers, > > > > Tom > > > > On 7 September 2017 at 16:42, Ismael Juma <ism...@juma.me.uk> wrote: > > > > > 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 > > > > > > > > > >
