kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ismael Juma <ism...@juma.me.uk>
Subject Re: [DISCUSS] KIP-195: AdminClient.increasePartitions
Date Fri, 08 Sep 2017 16:34:48 GMT
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.bentley@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.bentley@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 <yuzhihong@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 <ismael@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.bentley@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 <ppatierno@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.bentley@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.
> >> > > >
> >> > >
> >> >
> >>
> >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message