Hi Tom, A couple of comments:
1. "This API is synchronous in the sense that the client can assume that the partition count has been changed (or the request was rejected) once they have obtained the result for the topic from the CreatePartitionsResult." If you want to do this, I think you'd have to send the request to the Controller (which seems reasonable). The controller would then wait until the change has propagated to its metadata cache (like it does for create topics) before returning. 2. About using the create topics policy, I'm not sure. Aside from the naming issue, there's also the problem that the policy doesn't know if a creation or update is taking place. This matters because one may not want to allow the number of partitions to be changed after creation as it affects the semantics if keys are used. One option is to introduce a new interface that can be used by create, alter and delete with a new config. And deprecate CreateTopicPolicy. I doubt many are using it. What do you think? Ismael On Fri, Sep 8, 2017 at 5:34 PM, Ismael Juma <ism...@juma.me.uk> wrote: > Hi Tom, > > Maybe we should disallow the creation of partitions while a reassignment > is in progress. As you pointed out, there are weird edge cases. We can > relax the restriction later, if needed. > > Ismael > > On Fri, Sep 8, 2017 at 5:28 PM, Tom Bentley <t.j.bent...@gmail.com> wrote: > >> I've gone with createPartitions() and NewPartitions. >> >> To answer my own question about the replication factor, the current code >> (specifically AdminUtils.addPartitions()) just uses the size of the >> replica >> list of the first partition as the replication factor for the whole topic. >> I don't think this is 100% correct, for the reasons I mentioned earlier, >> but I propose to do the same thing in order to apply the >> CreateTopicPolicy. >> >> I'm very aware that I need to start the vote today if this is to have a >> hope of getting into 1.0.0, so I'm going to call the vote now. >> >> On 8 September 2017 at 16:28, Tom Bentley <t.j.bent...@gmail.com> wrote: >> >> > Putting aside the naming question for a moment, I have an implementation >> > question: >> > >> > I'm looking at applying the createTopicPolicy to the new partition >> count. >> > (As mentioned elsewhere, this is necessary to avoid creating a trivial >> > loophole in the policy where a user creates and then modifies a topic >> in a >> > way which would have violated the createTopicPolicy had the topic >> initially >> > been created with the new number of partitions.) >> > >> > To apply the policy I need the replication factor. But AFAICS the >> > configured replication factor isn't available anywhere on the broker. >> The >> > best I can do it look at size of the replicas list of a partition. But >> if >> > the partition is being reassigned this might not be the "real" >> replication >> > factor (it could be transiently higher). And yes, as far as I'm aware >> it is >> > possible to simultaneously increase the partition count and reassign the >> > existing partitions since they use different znodes. >> > >> > Also it seems that once the topic has been created it's possible to >> change >> > the replication factor of individual partitions by reassigning them. >> Which >> > is fine except it means there is no meaningful replication factor for >> the >> > *topic* after topic creation. If there's no meaningful replication >> factor I >> > can't apply the policy. >> > >> > Am I missing something? >> > >> > Thanks, >> > >> > Tom >> > >> > On 8 September 2017 at 13:50, Ted Yu <yuzhih...@gmail.com> wrote: >> > >> >> I think increasePartitions is better name, implying there are already >> >> partitions. >> >> >> >> bq. use cases for non-consecutive partition ids. >> >> >> >> +1 on not supporting non-consecutive partition ids >> >> >> >> Cheers >> >> >> >> On Fri, Sep 8, 2017 at 5:34 AM, Ismael Juma <ism...@juma.me.uk> wrote: >> >> >> >> > Hmm, maybe it should be createPartitions for symmetry with >> createTopics? >> >> > >> >> > Ismael >> >> > >> >> > On Fri, Sep 8, 2017 at 12:33 PM, Tom Bentley <t.j.bent...@gmail.com> >> >> > wrote: >> >> > >> >> > > Hi Paolo, >> >> > > >> >> > > Thanks for commenting. >> >> > > >> >> > > The main reason I suggested `NumPartitionsIncrease` rather than >> just >> >> > > `NumPartitions` was in case we ever implement a >> >> decreaseNumPartitions() >> >> > > API. The semantics of the class are not appropriate for using with >> a >> >> > > decrease API, but calling it NumPartitions suggests that it was >> >> related. >> >> > > >> >> > > Radical thought: What about if the method was called >> addPartitions() >> >> and >> >> > > the class was called NewPartitions? >> >> > > >> >> > > Then if an API for decreasing were ever implemented it could be >> >> > > removePartitions() with a RemovedPartitions class if necessary. >> >> > > >> >> > > Cheers, >> >> > > >> >> > > Tom >> >> > > >> >> > > On 8 September 2017 at 12:13, Paolo Patierno <ppatie...@live.com> >> >> wrote: >> >> > > >> >> > > > My 2 cents about naming ... >> >> > > > >> >> > > > >> >> > > > PartitionCount or NumPartitions sound better to me providing an >> >> > > "absolute" >> >> > > > value (as today the kafka-topics script work) for an idempotent >> >> > operation >> >> > > > while the NumPartitionsIncrease name sounds to me like the >> >> "increment" >> >> > > > value. >> >> > > > >> >> > > > >> >> > > > Paolo Patierno >> >> > > > Senior Software Engineer (IoT) @ Red Hat >> >> > > > Microsoft MVP on Windows Embedded & IoT >> >> > > > Microsoft Azure Advisor >> >> > > > >> >> > > > Twitter : @ppatierno<http://twitter.com/ppatierno> >> >> > > > Linkedin : paolopatierno<http://it.linkedin.com/in/paolopatierno >> > >> >> > > > Blog : DevExperience<http://paolopatierno.wordpress.com/> >> >> > > > >> >> > > > >> >> > > > ________________________________ >> >> > > > From: Tom Bentley <t.j.bent...@gmail.com> >> >> > > > Sent: Friday, September 8, 2017 9:39 AM >> >> > > > To: dev@kafka.apache.org >> >> > > > Subject: Re: [DISCUSS] KIP-195: AdminClient.increasePartitions >> >> > > > >> >> > > > Hi Ismael, >> >> > > > >> >> > > > Thanks for the comments. >> >> > > > >> >> > > > My bad for not noticing the custom assignment requirement. The >> >> current >> >> > > > > proposal has the following example (I updated it a little so >> that >> >> 2 >> >> > > > > partitions are added): >> >> > > > > >> >> > > > > increasePartitionCount(4, asList(asList(1, 2), asList(2, 3)) >> >> > > > > >> >> > > > > Why not simply provide the assignment? For example, if you >> want to >> >> > add >> >> > > 2 >> >> > > > > partitions, you'd simply do: >> >> > > > > >> >> > > > > increasePartitionCount(asList(asList(1, 2), asList(2, 3)) >> >> > > > > >> >> > > > > Not sure why need the number. >> >> > > > >> >> > > > >> >> > > > kafka-topics.sh allows to increase the number of partitions >> without >> >> > > > supplying an assignment, so one reason is simply to be able to >> >> support >> >> > > > that. >> >> > > > >> >> > > > When you don't supply an assignment you're leaving it to the >> >> cluster to >> >> > > > decide for you. By requiring an assignment you're forcing the >> user >> >> to >> >> > > > decide. The user might not care much and thus make a worse choice >> >> than >> >> > if >> >> > > > you'd left it to the server. >> >> > > > >> >> > > > >> >> > > > > The other two questions: >> >> > > > > >> >> > > > > 2. Do we want to allow people to add non-consecutive partition >> >> ids? >> >> > > This >> >> > > > is >> >> > > > > possible to do with the current AdminUtils, but it's not >> exactly >> >> > > > supported >> >> > > > > (although it apparently works fine in the broker). Still, I >> >> wanted to >> >> > > > make >> >> > > > > sure we considered it. >> >> > > > > >> >> > > > >> >> > > > I admit I had assumed this wasn't possible. How does partitioning >> >> work >> >> > if >> >> > > > there are holes? You would need the list of partition ids in >> order >> >> to >> >> > > > produce a correct partition id. >> >> > > > >> >> > > > Suspending my scepticism for a moment, to support something like >> >> that >> >> > > we'd >> >> > > > have to change the List<List<Integer>> assignment to a >> Map<Integer, >> >> > > > List<Integer>>, so the request explicitly identified the new >> topics, >> >> > > rather >> >> > > > than it being implied. That would make it slightly less easy to >> >> form a >> >> > > > valid request for the normal case of consecutive partition ids: >> >> You'd >> >> > > have >> >> > > > to actually know the current number of partitions, which might >> >> > > necessitate >> >> > > > a describeTopics(). >> >> > > > >> >> > > > It doesn't sound like there are any known use cases for >> >> non-consecutive >> >> > > > partition ids. It also sounds like whatever existing support >> there >> >> is >> >> > > might >> >> > > > be only lightly tested. It sounds like a source of gotchas and >> >> subtle >> >> > > bugs >> >> > > > to me (both in Kafka itself and for users). I have to wonder >> >> whether it >> >> > > > would be worth supporting this. >> >> > > > >> >> > > > If we decide not to support it, we should fix the rest of the >> >> > AdminClient >> >> > > > so it's not possible to create non-consecutive partition ids. >> >> > > > >> >> > > > WDYT? >> >> > > > >> >> > > > >> >> > > > 3. Do we want to provide the target partition count or the >> number we >> >> > want >> >> > > > > to increase it by? This is related to the first point as well. >> >> > Thinking >> >> > > > > about it, one benefit of specifying the target number is that >> the >> >> > > > operation >> >> > > > > is then idempotent. If we state the number we want to increase >> by, >> >> > it's >> >> > > > > easier to make a mistake and increase it twice under failure >> >> > scenarios. >> >> > > > Was >> >> > > > > that the motivation for specifying the target partition count? >> >> > > > > >> >> > > > > >> >> > > > Right, if you're just supplying an increment you can't be certain >> >> what >> >> > > > you're incrementing it to (which is what you actually care >> about). >> >> And >> >> > > > idempotency is so nice to have if something goes wrong. >> >> > > > >> >> > > > Using an increment would make the `NumPartitionIncrease` class a >> bit >> >> > more >> >> > > > easily understood, as then the outer list would have to be the >> same >> >> > size >> >> > > as >> >> > > > the increment. But for me idempotency is the more valuable >> feature. >> >> > > > >> >> > > >> >> > >> >> >> > >> > >> > >
