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