Thanks Ted, now fixed. On 13 December 2017 at 18:38, Ted Yu <yuzhih...@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.bent...@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 <j...@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 <j...@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.bent...@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 <j...@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 <j...@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.bent...@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 > > > >> > > > > > > > > > > > > > >
