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