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. >> > > > >> > > >> > >> > >
