kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Viktor Somogyi-Vass <viktorsomo...@gmail.com>
Subject Re: [DISCUSS] KIP-236 Interruptible Partition Reassignment
Date Fri, 22 Feb 2019 13:24:53 GMT
I've published the above mentioned KIP here:
https://cwiki.apache.org/confluence/display/KAFKA/KIP-435%3A+Incremental+Partition+Reassignment
Will start a discussion about it soon.

On Fri, Feb 22, 2019 at 12:45 PM Viktor Somogyi-Vass <
viktorsomogyi@gmail.com> wrote:

> Hi Folks,
>
> I also have a pending active work on the incremental partition
> reassignment stuff here: https://issues.apache.org/jira/browse/KAFKA-6794
> I think it would be good to cooperate on this to make both work compatible
> with each other.
>
> I'll write up a KIP about this today so it'll be easier to see how to fit
> the two together. Basically in my work I operate on the
> /admin/reassign_partitions node on a fully compatible way, meaning I won't
> change it just calculate each increment based on that and the current state
> of the ISR set for the partition in reassignment.
> I hope we could collaborate on this.
>
> Viktor
>
> On Thu, Feb 21, 2019 at 9:04 PM Harsha <kafka@harsha.io> wrote:
>
>> Thanks George. LGTM.
>> Jun & Tom, Can you please take a look at the updated KIP.
>> Thanks,
>> Harsha
>>
>> On Wed, Feb 20, 2019, at 12:18 PM, George Li wrote:
>> > Hi,
>> >
>> > After discussing with Tom, Harsha and I are picking up KIP-236 <
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-236%3A+Interruptible+Partition+Reassignment>.
>> The work focused on safely/cleanly cancel / rollback pending reassignments
>> in a timely fashion. Pull Request #6296 <
>> https://github.com/apache/kafka/pull/6296> Still working on more
>> integration/system tests.
>> >
>> > Please review and provide feedbacks/suggestions.
>> >
>> > Thanks,
>> > George
>> >
>> >
>> > On Saturday, December 23, 2017, 0:51:13 GMT, Jun Rao <jun@confluent.io>
>> wrote:
>> >
>> > Hi, Tom,
>>
>> Thanks for the reply.
>>
>> 10. That's a good thought. Perhaps it's better to get rid of
>> /admin/reassignment_requests
>> too. The window when a controller is not available is small. So, we can
>> just failed the admin client if the controller is not reachable after the
>> timeout.
>>
>> 13. With the changes in 10, the old approach is handled through ZK
>> callback
>> and the new approach is through Kafka RPC. The ordering between the two is
>> kind of arbitrary. Perhaps the ordering can just be based on the order
>> that
>> the reassignment is added to the controller request queue. From there, we
>> can either do the overriding or the prevention.
>>
>> Jun
>>
>>
>> On Fri, Dec 22, 2017 at 7:31 AM, Tom Bentley <t.j.bentley@gmail.com>
>> wrote:
>>
>> > Hi Jun,
>> >
>> > Thanks for responding, my replies are inline:
>> >
>> > 10. You explanation makes sense. My remaining concern is the additional
>> ZK
>> > > writes in the proposal. With the proposal, we will need to do
>> following
>> > > writes in ZK.
>> > >
>> > > a. write new assignment in /admin/reassignment_requests
>> > >
>> > > b. write new assignment and additional metadata in
>> > > /admin/reassignments/$topic/$partition
>> > >
>> > > c. write old + new assignment  in /brokers/topics/[topic]
>> > >
>> > > d. write new assignment in /brokers/topics/[topic]
>> > >
>> > > e. delete /admin/reassignments/$topic/$partition
>> > >
>> > > So, there are quite a few ZK writes. I am wondering if it's better to
>> > > consolidate the info in /admin/reassignments/$topic/$partition into
>> > > /brokers/topics/[topic].
>> > > For example, we can just add some new JSON fields in
>> > > /brokers/topics/[topic]
>> > > to remember the new assignment and potentially the original replica
>> count
>> > > when doing step c. Those fields with then be removed in step d. That
>> way,
>> > > we can get rid of step b and e, saving 2 ZK writes per partition.
>> > >
>> >
>> > This seems like a great idea to me.
>> >
>> > It might also be possible to get rid of the /admin/reassignment_requests
>> > subtree too. I've not yet published the ideas I have for the AdminClient
>> > API for reassigning partitions, but given the existence of such an API,
>> the
>> > route to starting a reassignment would be the AdminClient, and not
>> > zookeeper. In that case there is no need for
>> /admin/reassignment_requests
>> > at all. The only drawback that I can see is that while it's currently
>> > possible to trigger a reassignment even during a controller
>> > election/failover that would no longer be the case if all requests had
>> to
>> > go via the controller.
>> >
>> >
>> > > 11. What you described sounds good. We could potentially optimize the
>> > > dropped replicas a bit more. Suppose that assignment [0,1,2] is first
>> > > changed to [1,2,3] and then to [2,3,4]. When initiating the second
>> > > assignment, we may end up dropping replica 3 and only to restart it
>> > again.
>> > > In this case, we could only drop a replica if it's not going to be
>> added
>> > > back again.
>> > >
>> >
>> > I had missed that, thank you! I will update the proposed algorithm to
>> > prevent this.
>> >
>> >
>> > > 13. Since this is a corner case, we can either prevent or allow
>> > overriding
>> > > with old/new mechanisms. To me, it seems that allowing is simpler to
>> > > implement, the order in /admin/reassignment_requests determines the
>> > > ordering the of override, whether that's initiated by the new way or
>> the
>> > > old way.
>> > >
>> >
>> > That makes sense except for the corner case where:
>> >
>> > * There is no current controller and
>> > * Writes to both the new and old znodes happen
>> >
>> > On election of the new controller, for those partitions with both a
>> > reassignment_request and in /admin/reassign_partitions, we have to
>> decide
>> > which should win. You could use the modification time, though there are
>> > some very unlikely scenarios where that doesn't work properly, for
>> example
>> > if both znodes have the same mtime, or the /admin/reassign_partitions
>> was
>> > updated, but the assignment of the partition wasn't changed, like this:
>> >
>> > 0. /admin/reassign_partitions has my-topic/42 = [1,2,3]
>> > 1. Controller stops watching.
>> > 2. Create /admin/reassignment_requests/request_1234 to change the
>> > reassignment of partition my-topic/42 = [4,5,6]
>> > 3. Update /admin/reassign_partitions to add your-topic/12=[7,8,9]
>> > 4. New controller resumes
>> >
>> >
>> >
>> > > Thanks,
>> > >
>> > > Jun
>> > >
>> > > On Tue, Dec 19, 2017 at 2:43 AM, Tom Bentley <t.j.bentley@gmail.com>
>> > > wrote:
>> > >
>> > > > Hi Jun,
>> > > >
>> > > > 10. Another concern of mine is on consistency with the current
>> pattern.
>> > > The
>> > > > > current pattern for change notification based on ZK is (1) we
>> first
>> > > write
>> > > > > the actual value in the entity path and then write the change
>> > > > notification
>> > > > > path, and (2)  the change notification path only includes what
>> entity
>> > > has
>> > > > > changed but not the actual changes. If we want to follow this
>> pattern
>> > > for
>> > > > > consistency, /admin/reassignment_requests/request_xxx will only
>> have
>> > > the
>> > > > > partitions whose reassignment have changed, but not the actual
>> > > > > reassignment.
>> > > > >
>> > > >
>> > > > Ah, I hadn't understood part (2). That means my concern about
>> > efficiency
>> > > > with the current pattern is misplaced. There are still some
>> interesting
>> > > > differences in semantics, however:
>> > > >
>> > > > a) The mechanism currently proposed in KIP-236 means that the
>> > controller
>> > > is
>> > > > the only writer to /admin/reassignments. This means it can include
>> > > > information in these znodes that requesters might not know, or
>> > > information
>> > > > that's necessary to perform the reassignment but not necessary to
>> > > describe
>> > > > the request. While this could be handled using the current pattern
>> it
>> > > would
>> > > > rely on all  writers to preserve any information added by the
>> > controller,
>> > > > which seems complicated and hence fragile.
>> > > >
>> > > > b) The current pattern for change notification doesn't cope with
>> > > competing
>> > > > writers to the entity path: If two processes write to the entity
>> path
>> > > > before the controller can read it (due to notification) then one
>> set of
>> > > > updates will be lost.
>> > > >
>> > > > c) If a single writing process crashes after writing to the entity
>> > path,
>> > > > but before writing to the notification path then the write will be
>> > lost.
>> > > >
>> > > > I'm actually using point a) in my WIP (see below). Points b) and c)
>> are
>> > > > obviously edge cases.
>> > > >
>> > > >
>> > > > > 11. Ok. I am not sure that I fully understand the description of
>> that
>> > > > part.
>> > > > > Does "assigned" refer to the current assignment? Could you also
>> > > describe
>> > > > > where the length of the original assignment is stored in ZK?
>> > > > >
>> > > >
>> > > > Sorry if the description is not clear. Yes, "assigned" referrs to
>> the
>> > > > currently assigned replicas (taken from the
>> > > > ControllerContext.partitionReplicaAssignment). I would store the
>> > length
>> > > of
>> > > > the original assignment in the
>> /admin/reassignments/$topic/$partition
>> > > > znode
>> > > > (this is where the point (a) above is useful -- the requester
>> shouldn't
>> > > > know that this information is used by the controller).
>> > > >
>> > > > I've updated the KIP to make these points clearer.
>> > > >
>> > > >
>> > > > > 13. Hmm, I am not sure that the cancellation needs to be done for
>> the
>> > > > whole
>> > > > > batch. The reason that I brought this up is for consistency. The
>> KIP
>> > > > allows
>> > > > > override when using the new approach. It just seems that it's
>> simpler
>> > > to
>> > > > > extend this model when resolving multiple changes between the old
>> and
>> > > the
>> > > > > new approach.
>> > > >
>> > > >
>> > > > Ah, I think I've been unclear on this point too. Currently the
>> > > > ReassignPartitionsCommand enforces that you can't change
>> reassignments,
>> > > but
>> > > > this doesn't stop other ZK clients making changes to
>> > > > /admin/reassign_partitions directly and I believe some Kafka users
>> do
>> > > > indeed change reassignments in-flight by writing to
>> > > > /admin/reassign_partitions. What I'm proposing doesn't break that at
>> > all.
>> > > > The semantic I've implemented is only that the controller only
>> refuses
>> > a
>> > > > reassignment change if there is already one in-flight (i.e. in
>> > > > /admin/reassignments/$topic/$partition) **via the other mechansim**.
>> > So
>> > > if
>> > > > you're using /admin/reassign_partitions and you change or cancel
>> part
>> > of
>> > > it
>> > > > via /admin/reassign_partitions, that's OK. Likewise if you're using
>> > > > /admin/reassignment_request/request_xxx and you change or cancel
>> part
>> > of
>> > > > it
>> > > > via another /admin/reassignment_request/request_xxx, that's OK.What
>> > you
>> > > > can't do is change a request that was started via
>> > > > /admin/reassign_partitions via /admin/reassignment_request/
>> > request_xxx,
>> > > or
>> > > > vice versa.
>> > > >
>> > > > What I was thinking of when I replied is the case where, on
>> controller
>> > > > failover, /admin/reassign_partitions has been changed and
>> > > > /admin/reassignment_request/request_xxx created (in the period when
>> > the
>> > > > new
>> > > > controller was being elected, for example) with a common partition.
>> In
>> > > this
>> > > > case we should apply a consistent rule to that used when the
>> > notification
>> > > > happen in real time. Your suggestion to use the modification time of
>> > the
>> > > > znode would work here too (except in the edge case where ZK writes
>> to
>> > > both
>> > > > znodes happen within the same clock tick on the ZK server, so the
>> > mtimes
>> > > > are the same).
>> > > >
>> > > > Let me know if you think this is the right semantic and I'll try to
>> > > clarify
>> > > > the KIP.
>> > > >
>> > > > Many thanks,
>> > > >
>> > > > Tom
>> > > >
>> > > > On 18 December 2017 at 18:12, Jun Rao <jun@confluent.io> wrote:
>> > > >
>> > > > > Hi, Tom,
>> > > > >
>> > > > > Thanks for the reply. A few more followup comments below.
>> > > > >
>> > > > > 10. Another concern of mine is on consistency with the current
>> > pattern.
>> > > > The
>> > > > > current pattern for change notification based on ZK is (1) we
>> first
>> > > write
>> > > > > the actual value in the entity path and then write the change
>> > > > notification
>> > > > > path, and (2)  the change notification path only includes what
>> entity
>> > > has
>> > > > > changed but not the actual changes. If we want to follow this
>> pattern
>> > > for
>> > > > > consistency, /admin/reassignment_requests/request_xxx will only
>> have
>> > > the
>> > > > > partitions whose reassignment have changed, but not the actual
>> > > > > reassignment.
>> > > > >
>> > > > > 11. Ok. I am not sure that I fully understand the description of
>> that
>> > > > part.
>> > > > > Does "assigned" refer to the current assignment? Could you also
>> > > describe
>> > > > > where the length of the original assignment is stored in ZK?
>> > > > >
>> > > > > 13. Hmm, I am not sure that the cancellation needs to be done for
>> the
>> > > > whole
>> > > > > batch. The reason that I brought this up is for consistency. The
>> KIP
>> > > > allows
>> > > > > override when using the new approach. It just seems that it's
>> simpler
>> > > to
>> > > > > extend this model when resolving multiple changes between the old
>> and
>> > > the
>> > > > > new approach.
>> > > > >
>> > > > > Jun
>> > > > >
>> > > > >
>> > > > >
>> > > > > On Mon, Dec 18, 2017 at 2:45 AM, Tom Bentley <
>> t.j.bentley@gmail.com>
>> > > > > wrote:
>> > > > >
>> > > > > > Hi Jun,
>> > > > > >
>> > > > > > Thanks for replying, some answers below:
>> > > > > >
>> > > > > >
>> > > > > > > 10. The proposal now stores the reassignment for all
>> partitions
>> > in
>> > > > > > > /admin/reassignment_requests/request_xxx. If the number of
>> > > > reassigned
>> > > > > > > partitions is larger, the ZK write may hit the default 1MB
>> limit
>> > > and
>> > > > > > fail.
>> > > > > > > An alternative approach is to have the reassignment requester
>> > first
>> > > > > write
>> > > > > > > the new assignment for each partition under
>> > > > > > > /admin/reassignments/$topic/$partition and then write
>> > > > > > > /admin/reassignment_requests/request_xxx with an empty value.
>> > The
>> > > > > > > controller can then read all values under
>> /admin/reassignments.
>> > > > > > >
>> > > > > >
>> > > > > > You're right that reassigning enough partitions would hit the
>> 1MB
>> > > > limit,
>> > > > > > but I don't think this would be a problem in practice because it
>> > > would
>> > > > be
>> > > > > > trivial to split the partitions into several requests (i.e.
>> > mutleiple
>> > > > > > request_xxx).
>> > > > > > I don't think the non-atomicity this would imply is a problem.
>> By
>> > > > writing
>> > > > > > the partitions whose /admin/reassignments/$topic/$partition has
>> > been
>> > > > > > created or changed it makes it much more efficient to know
>> which of
>> > > > those
>> > > > > > znodes we need to read. If I understand your suggestion, you
>> would
>> > > have
>> > > > > to
>> > > > > > read every node under /admin/reassignments to figure out which
>> had
>> > > > > changed.
>> > > > > >
>> > > > > >
>> > > > > > > 11. The improvement you suggested in onPartitionReassignment()
>> > > sounds
>> > > > > > good
>> > > > > > > at the high level. The computation of those dropped partitions
>> > > seems
>> > > > a
>> > > > > > bit
>> > > > > > > complicated. Perhaps a simple approach is to drop the replicas
>> > not
>> > > in
>> > > > > the
>> > > > > > > original assignment and newest reassignment?
>> > > > > > >
>> > > > > >
>> > > > > > This was what I came up with originally too, but when I looked
>> into
>> > > > > > implementing it I found a couple of things which made me
>> reconsider
>> > > it.
>> > > > > > Consider the reassignments [0,1] -> [2,3] -> [3,4]. In words: we
>> > > start
>> > > > > > reassigning to [2,3], but then change our minds about 2 and
>> switch
>> > it
>> > > > to
>> > > > > 4
>> > > > > > (maybe we've figured out a better overall balance). At that
>> point
>> > it
>> > > is
>> > > > > > perfectly possible that broker 2 is in-sync and broker 1 is not
>> > > > in-sync.
>> > > > > It
>> > > > > > seems silly to drop broker 2 in favour of broker 1: We're
>> > needlessly
>> > > > > giving
>> > > > > > the cluster more work to do.
>> > > > > >
>> > > > > > The second thing that made me reconsider was in that same
>> scenario
>> > > it's
>> > > > > > even possible that broker 2 is the leader of the partition.
>> > Obviously
>> > > > we
>> > > > > > can elect a new leader before dropping it, but not without
>> causing
>> > > > > > disruption to producers and consumers.
>> > > > > >
>> > > > > > By accepting a little more complexity in choosing which brokers
>> to
>> > > drop
>> > > > > we
>> > > > > > make the dropping simpler (no need for leader election) and
>> ensure
>> > > the
>> > > > > > cluster has less work to do.
>> > > > > >
>> > > > > >
>> > > > > > > 12. You brought up the need of remembering the original
>> > assignment.
>> > > > > This
>> > > > > > > will be lost if the assignment is changed multiple times if we
>> > > follow
>> > > > > the
>> > > > > > > approach described in 10. One way is to store the original
>> > > assignment
>> > > > > in
>> > > > > > > /brokers/topics/[topic] as the following. When the final
>> > > reassignment
>> > > > > > > completes, we can remove the original field.
>> > > > > > > {
>> > > > > > >   "version": 1,
>> > > > > > >   "partitions": {"0": [0, 1, 3] },
>> > > > > > >   "originals": {"0": [0, 1, 2] }
>> > > > > > > }
>> > > > > > >
>> > > > > >
>> > > > > > While I was implementing my first version of
>> > > onPartitionReassignment(),
>> > > > > > where I preferred the originals, I was storing the originals in
>> the
>> > > > > > /admin/reassignments/$topic/$partition znodes. Since we will
>> > remove
>> > > > that
>> > > > > > znode at the end of reassignment anyway, I would suggest this
>> is a
>> > > > better
>> > > > > > place to store that data (if it's necessary to do so), so that
>> we
>> > can
>> > > > > avoid
>> > > > > > another ZK round trip.
>> > > > > >
>> > > > > >
>> > > > > > > 13. For resolving the conflict between
>> /admin/reassign_partitions
>> > > and
>> > > > > > > /admin/reassignments/$topic/$partition, perhaps it's more
>> > natural
>> > > to
>> > > > > > just
>> > > > > > > let the assignment with a newer timestamp to override the
>> older
>> > > one?
>> > > > > > >
>> > > > > >
>> > > > > > That would work but with slightly different semantics to what I
>> > have:
>> > > > > Since
>> > > > > > /admin/reassign_partitions contains multiple partitions, using
>> the
>> > > > > > timestamp means the whole batch wins or losses. By tracking how
>> > each
>> > > > > > request was made we can be more fine-grained. I'm to use the
>> > > > modification
>> > > > > > time if such granularity is not required.
>> > > > > >
>> > > > > >
>> > > > > > > 14. Implementation wise, currently, we register a watcher of
>> the
>> > > isr
>> > > > > path
>> > > > > > > of each partition being reassigned. This has the potential
>> issue
>> > of
>> > > > > > > registering many listeners. An improvement could be just
>> > > piggybacking
>> > > > > on
>> > > > > > > the existing IsrChangeNotificationHandler, which only watches
>> a
>> > > > single
>> > > > > ZK
>> > > > > > > path and is triggered on a batch of isr changes. This is kind
>> of
>> > > > > > orthogonal
>> > > > > > > to the KIP. However, if we are touching the reassignment
>> logic,
>> > it
>> > > > may
>> > > > > be
>> > > > > > > worth considering.
>> > > > > >
>> > > > > >
>> > > > > > Let me look into that.
>> > > > > >
>> > > > > > Thanks,
>> > > > > >
>> > > > > > Tom
>> > > > > >
>> > > > > > On 16 December 2017 at 02:19, Jun Rao <jun@confluent.io> wrote:
>> > > > > >
>> > > > > > > Hi, Tom,
>> > > > > > >
>> > > > > > > Thanks for the updated KIP. A few more comments below.
>> > > > > > >
>> > > > > > > 10. The proposal now stores the reassignment for all
>> partitions
>> > in
>> > > > > > > /admin/reassignment_requests/request_xxx. If the number of
>> > > > reassigned
>> > > > > > > partitions is larger, the ZK write may hit the default 1MB
>> limit
>> > > and
>> > > > > > fail.
>> > > > > > > An alternative approach is to have the reassignment requester
>> > first
>> > > > > write
>> > > > > > > the new assignment for each partition under
>> > > > > > > /admin/reassignments/$topic/$partition and then write
>> > > > > > > /admin/reassignment_requests/request_xxx with an empty value.
>> > The
>> > > > > > > controller can then read all values under
>> /admin/reassignments.
>> > > > > > >
>> > > > > > > 11. The improvement you suggested in onPartitionReassignment()
>> > > sounds
>> > > > > > good
>> > > > > > > at the high level. The computation of those dropped partitions
>> > > seems
>> > > > a
>> > > > > > bit
>> > > > > > > complicated. Perhaps a simple approach is to drop the replicas
>> > not
>> > > in
>> > > > > the
>> > > > > > > original assignment and newest reassignment?
>> > > > > > >
>> > > > > > > 12. You brought up the need of remembering the original
>> > assignment.
>> > > > > This
>> > > > > > > will be lost if the assignment is changed multiple times if we
>> > > follow
>> > > > > the
>> > > > > > > approach described in 10. One way is to store the original
>> > > assignment
>> > > > > in
>> > > > > > > /brokers/topics/[topic] as the following. When the final
>> > > reassignment
>> > > > > > > completes, we can remove the original field.
>> > > > > > > {
>> > > > > > >   "version": 1,
>> > > > > > >   "partitions": {"0": [0, 1, 3] },
>> > > > > > >   "originals": {"0": [0, 1, 2] }
>> > > > > > > }
>> > > > > > >
>> > > > > > > 13. For resolving the conflict between
>> /admin/reassign_partitions
>> > > and
>> > > > > > > /admin/reassignments/$topic/$partition, perhaps it's more
>> > natural
>> > > to
>> > > > > > just
>> > > > > > > let the assignment with a newer timestamp to override the
>> older
>> > > one?
>> > > > > > >
>> > > > > > > 14. Implementation wise, currently, we register a watcher of
>> the
>> > > isr
>> > > > > path
>> > > > > > > of each partition being reassigned. This has the potential
>> issue
>> > of
>> > > > > > > registering many listeners. An improvement could be just
>> > > piggybacking
>> > > > > on
>> > > > > > > the existing IsrChangeNotificationHandler, which only watches
>> a
>> > > > single
>> > > > > ZK
>> > > > > > > path and is triggered on a batch of isr changes. This is kind
>> of
>> > > > > > orthogonal
>> > > > > > > to the KIP. However, if we are touching the reassignment
>> logic,
>> > it
>> > > > may
>> > > > > be
>> > > > > > > worth considering.
>> > > > > > >
>> > > > > > > Thanks,
>> > > > > > >
>> > > > > > > Jun
>> > > > > > >
>> > > > > > > On Fri, Dec 15, 2017 at 10:17 AM, Tom Bentley <
>> > > t.j.bentley@gmail.com
>> > > > >
>> > > > > > > wrote:
>> > > > > > >
>> > > > > > > > Just wanted to mention that I've started KIP-240, which
>> builds
>> > on
>> > > > top
>> > > > > > of
>> > > > > > > > this one to provide an AdminClient API for listing and
>> > describing
>> > > > > > > > reassignments.
>> > > > > > > >
>> > > > > > > > On 15 December 2017 at 14:34, Tom Bentley <
>> > t.j.bentley@gmail.com
>> > > >
>> > > > > > wrote:
>> > > > > > > >
>> > > > > > > > > > Should we seek to improve this algorithm in this KIP, or
>> > > leave
>> > > > > that
>> > > > > > > as
>> > > > > > > > > a later optimisation?
>> > > > > > > > >
>> > > > > > > > > I've updated the KIP with a proposed algorithm.
>> > > > > > > > >
>> > > > > > > > >
>> > > > > > > > >
>> > > > > > > > >
>> > > > > > > > >
>> > > > > > > > >
>> > > > > > > > > On 14 December 2017 at 09:57, Tom Bentley <
>> > > t.j.bentley@gmail.com
>> > > > >
>> > > > > > > wrote:
>> > > > > > > > >
>> > > > > > > > >> Thanks Ted, now fixed.
>> > > > > > > > >>
>> > > > > > > > >> On 13 December 2017 at 18:38, Ted Yu <
>> yuzhihong@gmail.com>
>> > > > wrote:
>> > > > > > > > >>
>> > > > > > > > >>> Tom:
>> > > > > > > > >>> bq. create a znode
>> /admin/reassignments/$topic-$partition
>> > > > > > > > >>>
>> > > > > > > > >>> Looks like the tree structure above should be:
>> > > > > > > > >>>
>> > > > > > > > >>> /admin/reassignments/$topic/$partition
>> > > > > > > > >>>
>> > > > > > > > >>> bq. The controller removes /admin/reassignment/$topic/$
>> > > > partition
>> > > > > > > > >>>
>> > > > > > > > >>> Note the lack of 's' for reassignment. It would be good
>> to
>> > > make
>> > > > > > > > zookeeper
>> > > > > > > > >>> paths consistent.
>> > > > > > > > >>>
>> > > > > > > > >>> Thanks
>> > > > > > > > >>>
>> > > > > > > > >>> On Wed, Dec 13, 2017 at 9:49 AM, Tom Bentley <
>> > > > > > t.j.bentley@gmail.com>
>> > > > > > > > >>> wrote:
>> > > > > > > > >>>
>> > > > > > > > >>> > Hi Jun and Ted,
>> > > > > > > > >>> >
>> > > > > > > > >>> > Jun, you're right that needing one watcher per
>> reassigned
>> > > > > > partition
>> > > > > > > > >>> > presents a scalability problem, and using a separate
>> > > > > notification
>> > > > > > > > path
>> > > > > > > > >>> > solves that. I also agree that it makes sense to
>> prevent
>> > > > users
>> > > > > > from
>> > > > > > > > >>> using
>> > > > > > > > >>> > both methods on the same reassignment.
>> > > > > > > > >>> >
>> > > > > > > > >>> > Ted, naming the reassignments like mytopic-42 was
>> simpler
>> > > > > while I
>> > > > > > > was
>> > > > > > > > >>> > proposing a watcher-per-reassignment (I'd have needed
>> a
>> > > child
>> > > > > > > watcher
>> > > > > > > > >>> on
>> > > > > > > > >>> > /admin/reassignments and also on
>> > > > /admin/reassignments/mytopic).
>> > > > > > > Using
>> > > > > > > > >>> the
>> > > > > > > > >>> > separate notification path means I don't need any
>> > watchers
>> > > in
>> > > > > the
>> > > > > > > > >>> > /admin/reassignments subtree, so switching to
>> > > > > > > > >>> /admin/reassignments/mytopic/
>> > > > > > > > >>> > 42
>> > > > > > > > >>> > would work, and avoid /admin/reassignments having a
>> very
>> > > > large
>> > > > > > > number
>> > > > > > > > >>> of
>> > > > > > > > >>> > child nodes. On the other hand it also means I have to
>> > > create
>> > > > > and
>> > > > > > > > >>> delete
>> > > > > > > > >>> > the topic nodes (e.g. /admin/reassignments/mytopic),
>> > which
>> > > > > incurs
>> > > > > > > the
>> > > > > > > > >>> cost
>> > > > > > > > >>> > of extra round trips to zookeeper. I suppose that
>> since
>> > > > > > > reassignment
>> > > > > > > > is
>> > > > > > > > >>> > generally a slow process it makes little difference
>> if we
>> > > > > > increase
>> > > > > > > > the
>> > > > > > > > >>> > latency of the interactions with zookeeper.
>> > > > > > > > >>> >
>> > > > > > > > >>> > I have updated the KIP with these improvements, and a
>> > more
>> > > > > > detailed
>> > > > > > > > >>> > description of exactly how we would manage these
>> znodes.
>> > > > > > > > >>> >
>> > > > > > > > >>> > Reading the algorithm in KafkaController.
>> > > > > > > onPartitionReassignment(),
>> > > > > > > > it
>> > > > > > > > >>> > seems that it would be suboptimal for changing
>> > > reassignments
>> > > > > > > > in-flight.
>> > > > > > > > >>> > Consider an initial assignment of [1,2], reassigned to
>> > > [2,3]
>> > > > > and
>> > > > > > > then
>> > > > > > > > >>> > changed to [2,4]. Broker 3 will remain in the assigned
>> > > > replicas
>> > > > > > > until
>> > > > > > > > >>> > broker 4 is in sync, even though 3 wasn't actually
>> one of
>> > > the
>> > > > > > > > original
>> > > > > > > > >>> > assigned replicas and is no longer a new assigned
>> > replica.
>> > > I
>> > > > > > think
>> > > > > > > > this
>> > > > > > > > >>> > also affects the case where the reassignment is
>> cancelled
>> > > > > > > > >>> > ([1,2]->[2,3]->[1,2]): We again have to wait for 3 to
>> > catch
>> > > > up,
>> > > > > > > even
>> > > > > > > > >>> though
>> > > > > > > > >>> > its replica will then be deleted.
>> > > > > > > > >>> >
>> > > > > > > > >>> > Should we seek to improve this algorithm in this KIP,
>> or
>> > > > leave
>> > > > > > that
>> > > > > > > > as
>> > > > > > > > >>> a
>> > > > > > > > >>> > later optimisation?
>> > > > > > > > >>> >
>> > > > > > > > >>> > Cheers,
>> > > > > > > > >>> >
>> > > > > > > > >>> > Tom
>> > > > > > > > >>> >
>> > > > > > > > >>> > On 11 December 2017 at 21:31, Jun Rao <
>> jun@confluent.io>
>> > > > > wrote:
>> > > > > > > > >>> >
>> > > > > > > > >>> > > Another question is on the compatibility. Since now
>> > there
>> > > > > are 2
>> > > > > > > > ways
>> > > > > > > > >>> of
>> > > > > > > > >>> > > specifying a partition reassignment, one under
>> > > > > > > > >>> /admin/reassign_partitions
>> > > > > > > > >>> > > and the other under /admin/reassignments, we
>> probably
>> > > want
>> > > > to
>> > > > > > > > >>> prevent the
>> > > > > > > > >>> > > same topic being reassigned under both paths at the
>> > same
>> > > > > time?
>> > > > > > > > >>> > > Thanks,
>> > > > > > > > >>> > >
>> > > > > > > > >>> > > Jun
>> > > > > > > > >>> > >
>> > > > > > > > >>> > >
>> > > > > > > > >>> > >
>> > > > > > > > >>> > > On Fri, Dec 8, 2017 at 5:41 PM, Jun Rao <
>> > > jun@confluent.io>
>> > > > > > > wrote:
>> > > > > > > > >>> > >
>> > > > > > > > >>> > > > Hi, Tom,
>> > > > > > > > >>> > > >
>> > > > > > > > >>> > > > Thanks for the KIP. It definitely addresses one of
>> > the
>> > > > pain
>> > > > > > > > points
>> > > > > > > > >>> in
>> > > > > > > > >>> > > > partition reassignment. Another issue that it also
>> > > > > addresses
>> > > > > > is
>> > > > > > > > >>> the ZK
>> > > > > > > > >>> > > node
>> > > > > > > > >>> > > > size limit when writing the reassignment JSON.
>> > > > > > > > >>> > > >
>> > > > > > > > >>> > > > My only concern is that the KIP needs to create
>> one
>> > > > watcher
>> > > > > > per
>> > > > > > > > >>> > > reassigned
>> > > > > > > > >>> > > > partition. This could add overhead in ZK and
>> > complexity
>> > > > for
>> > > > > > > > >>> debugging
>> > > > > > > > >>> > > when
>> > > > > > > > >>> > > > lots of partitions are being reassigned
>> > simultaneously.
>> > > > We
>> > > > > > > could
>> > > > > > > > >>> > > > potentially improve this by introducing a
>> separate ZK
>> > > > path
>> > > > > > for
>> > > > > > > > >>> change
>> > > > > > > > >>> > > > notification as we do for configs. For example,
>> every
>> > > > time
>> > > > > we
>> > > > > > > > >>> change
>> > > > > > > > >>> > the
>> > > > > > > > >>> > > > assignment for a set of partitions, we could
>> further
>> > > > write
>> > > > > a
>> > > > > > > > >>> sequential
>> > > > > > > > >>> > > > node /admin/reassignment_changes/[change_x]. That
>> > way,
>> > > > the
>> > > > > > > > >>> controller
>> > > > > > > > >>> > > > only needs to watch the change path. Once a
>> change is
>> > > > > > > triggered,
>> > > > > > > > >>> the
>> > > > > > > > >>> > > > controller can read everything under
>> > > > /admin/reassignments/.
>> > > > > > > > >>> > > >
>> > > > > > > > >>> > > > Jun
>> > > > > > > > >>> > > >
>> > > > > > > > >>> > > >
>> > > > > > > > >>> > > > On Wed, Dec 6, 2017 at 1:19 PM, Tom Bentley <
>> > > > > > > > t.j.bentley@gmail.com
>> > > > > > > > >>> >
>> > > > > > > > >>> > > wrote:
>> > > > > > > > >>> > > >
>> > > > > > > > >>> > > >> Hi,
>> > > > > > > > >>> > > >>
>> > > > > > > > >>> > > >> This is still very new, but I wanted some quick
>> > > feedback
>> > > > > on
>> > > > > > a
>> > > > > > > > >>> > > preliminary
>> > > > > > > > >>> > > >> KIP which could, I think, help with providing an
>> > > > > AdminClient
>> > > > > > > API
>> > > > > > > > >>> for
>> > > > > > > > >>> > > >> partition reassignment.
>> > > > > > > > >>> > > >>
>> > > > > > > > >>> > > >> https://cwiki.apache.org/
>> > > confluence/display/KAFKA/KIP-
>> > > > > 236%
>> > > > > > > > >>> > > >> 3A+Interruptible+Partition+Reassignment
>> > > > > > > > >>> > > >>
>> > > > > > > > >>> > > >> I wasn't sure whether to start fleshing out a
>> whole
>> > > > > > > AdminClient
>> > > > > > > > >>> API in
>> > > > > > > > >>> > > >> this
>> > > > > > > > >>> > > >> KIP (which would make it very big, and difficult
>> to
>> > > > read),
>> > > > > > or
>> > > > > > > > >>> whether
>> > > > > > > > >>> > to
>> > > > > > > > >>> > > >> break it down into smaller KIPs (which makes it
>> > easier
>> > > > to
>> > > > > > read
>> > > > > > > > and
>> > > > > > > > >>> > > >> implement in pieces, but harder to get a
>> high-level
>> > > > > picture
>> > > > > > of
>> > > > > > > > the
>> > > > > > > > >>> > > >> ultimate
>> > > > > > > > >>> > > >> destination). For now I've gone for a very small
>> > > initial
>> > > > > > KIP,
>> > > > > > > > but
>> > > > > > > > >>> I'm
>> > > > > > > > >>> > > >> happy
>> > > > > > > > >>> > > >> to sketch the bigger picture here if people are
>> > > > > interested.
>> > > > > > > > >>> > > >>
>> > > > > > > > >>> > > >> Cheers,
>> > > > > > > > >>> > > >>
>> > > > > > > > >>> > > >> Tom
>> > > > > > > > >>> > > >>
>> > > > > > > > >>> > > >
>> > > > > > > > >>> > > >
>> > > > > > > > >>> > >
>> > > > > > > > >>> >
>> > > > > > > > >>> >
>> > > > > > > > >>> > On 11 December 2017 at 21:31, Jun Rao <
>> jun@confluent.io>
>> > > > > wrote:
>> > > > > > > > >>> >
>> > > > > > > > >>> > > Another question is on the compatibility. Since now
>> > there
>> > > > > are 2
>> > > > > > > > ways
>> > > > > > > > >>> of
>> > > > > > > > >>> > > specifying a partition reassignment, one under
>> > > > > > > > >>> /admin/reassign_partitions
>> > > > > > > > >>> > > and the other under /admin/reassignments, we
>> probably
>> > > want
>> > > > to
>> > > > > > > > >>> prevent the
>> > > > > > > > >>> > > same topic being reassigned under both paths at the
>> > same
>> > > > > time?
>> > > > > > > > >>> > > Thanks,
>> > > > > > > > >>> > >
>> > > > > > > > >>> > > Jun
>> > > > > > > > >>> > >
>> > > > > > > > >>> > >
>> > > > > > > > >>> > >
>> > > > > > > > >>> > > On Fri, Dec 8, 2017 at 5:41 PM, Jun Rao <
>> > > jun@confluent.io>
>> > > > > > > wrote:
>> > > > > > > > >>> > >
>> > > > > > > > >>> > > > Hi, Tom,
>> > > > > > > > >>> > > >
>> > > > > > > > >>> > > > Thanks for the KIP. It definitely addresses one of
>> > the
>> > > > pain
>> > > > > > > > points
>> > > > > > > > >>> in
>> > > > > > > > >>> > > > partition reassignment. Another issue that it also
>> > > > > addresses
>> > > > > > is
>> > > > > > > > >>> the ZK
>> > > > > > > > >>> > > node
>> > > > > > > > >>> > > > size limit when writing the reassignment JSON.
>> > > > > > > > >>> > > >
>> > > > > > > > >>> > > > My only concern is that the KIP needs to create
>> one
>> > > > watcher
>> > > > > > per
>> > > > > > > > >>> > > reassigned
>> > > > > > > > >>> > > > partition. This could add overhead in ZK and
>> > complexity
>> > > > for
>> > > > > > > > >>> debugging
>> > > > > > > > >>> > > when
>> > > > > > > > >>> > > > lots of partitions are being reassigned
>> > simultaneously.
>> > > > We
>> > > > > > > could
>> > > > > > > > >>> > > > potentially improve this by introducing a
>> separate ZK
>> > > > path
>> > > > > > for
>> > > > > > > > >>> change
>> > > > > > > > >>> > > > notification as we do for configs. For example,
>> every
>> > > > time
>> > > > > we
>> > > > > > > > >>> change
>> > > > > > > > >>> > the
>> > > > > > > > >>> > > > assignment for a set of partitions, we could
>> further
>> > > > write
>> > > > > a
>> > > > > > > > >>> sequential
>> > > > > > > > >>> > > > node /admin/reassignment_changes/[change_x]. That
>> > way,
>> > > > the
>> > > > > > > > >>> controller
>> > > > > > > > >>> > > > only needs to watch the change path. Once a
>> change is
>> > > > > > > triggered,
>> > > > > > > > >>> the
>> > > > > > > > >>> > > > controller can read everything under
>> > > > /admin/reassignments/.
>> > > > > > > > >>> > > >
>> > > > > > > > >>> > > > Jun
>> > > > > > > > >>> > > >
>> > > > > > > > >>> > > >
>> > > > > > > > >>> > > > On Wed, Dec 6, 2017 at 1:19 PM, Tom Bentley <
>> > > > > > > > t.j.bentley@gmail.com
>> > > > > > > > >>> >
>> > > > > > > > >>> > > wrote:
>> > > > > > > > >>> > > >
>> > > > > > > > >>> > > >> Hi,
>> > > > > > > > >>> > > >>
>> > > > > > > > >>> > > >> This is still very new, but I wanted some quick
>> > > feedback
>> > > > > on
>> > > > > > a
>> > > > > > > > >>> > > preliminary
>> > > > > > > > >>> > > >> KIP which could, I think, help with providing an
>> > > > > AdminClient
>> > > > > > > API
>> > > > > > > > >>> for
>> > > > > > > > >>> > > >> partition reassignment.
>> > > > > > > > >>> > > >>
>> > > > > > > > >>> > > >> https://cwiki.apache.org/
>> > > confluence/display/KAFKA/KIP-
>> > > > > 236%
>> > > > > > > > >>> > > >> 3A+Interruptible+Partition+Reassignment
>> > > > > > > > >>> > > >>
>> > > > > > > > >>> > > >> I wasn't sure whether to start fleshing out a
>> whole
>> > > > > > > AdminClient
>> > > > > > > > >>> API in
>> > > > > > > > >>> > > >> this
>> > > > > > > > >>> > > >> KIP (which would make it very big, and difficult
>> to
>> > > > read),
>> > > > > > or
>> > > > > > > > >>> whether
>> > > > > > > > >>> > to
>> > > > > > > > >>> > > >> break it down into smaller KIPs (which makes it
>> > easier
>> > > > to
>> > > > > > read
>> > > > > > > > and
>> > > > > > > > >>> > > >> implement in pieces, but harder to get a
>> high-level
>> > > > > picture
>> > > > > > of
>> > > > > > > > the
>> > > > > > > > >>> > > >> ultimate
>> > > > > > > > >>> > > >> destination). For now I've gone for a very small
>> > > initial
>> > > > > > KIP,
>> > > > > > > > but
>> > > > > > > > >>> I'm
>> > > > > > > > >>> > > >> happy
>> > > > > > > > >>> > > >> to sketch the bigger picture here if people are
>> > > > > interested.
>> > > > > > > > >>> > > >>
>> > > > > > > > >>> > > >> Cheers,
>> > > > > > > > >>> > > >>
>> > > > > > > > >>> > > >> Tom
>> > > > > > > > >>> > > >>
>> > > > > > > > >>> > > >
>> > > > > > > > >>> > > >
>> > > > > > > > >>> > >
>> > > > > > > > >>> >
>> > > > > > > > >>>
>> > > > > > > > >>
>> > > > > > > > >>
>> > > > > > > > >
>> > > > > > > >
>> > > > > > >
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>> >
>>
>

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