+1 I agree with Guozhang that broker epoch will need a separate discussion.
Thanks! Jason On Thu, May 28, 2020 at 9:34 AM Guozhang Wang <wangg...@gmail.com> wrote: > David, thanks for the KIP. I'm +1 on it as well. > > One note is that in post-ZK world, we would need a different way to get > broker epoch since it is updated as ZKversion today. I believe we would > have this discussion in a different KIP though. > > > Guozhang > > On Wed, May 27, 2020 at 8:26 PM Colin McCabe <cmcc...@apache.org> wrote: > > > Thanks, David. +1 (binding). > > > > cheers, > > Colin > > > > On Wed, May 27, 2020, at 18:21, David Arthur wrote: > > > Colin, thanks for the feedback. Good points. I've updated the KIP with > > your > > > suggestions. > > > > > > -David > > > > > > On Wed, May 27, 2020 at 4:05 PM Colin McCabe <cmcc...@apache.org> > wrote: > > > > > > > Hi David, > > > > > > > > Thanks for the KIP! > > > > > > > > The KIP refers to "the KIP-500 bridge release (version 2.6.0 as of > the > > > > time of this proposal)". This is out of date-- the bridge release > > will be > > > > one of the 3.x releases. We should either update this to 3.0, or > > perhaps > > > > just take out the reference to a specific version, since it's not > > necessary > > > > to understand the rest of the KIP. > > > > > > > > > ... and potentially could replace the existing controlled shutdown > > RPC. > > > > Since this RPC > > > > > is somewhat generic, it could also be used to mark a replicas a > > "online" > > > > following some > > > > > kind of log dir recovery procedure (out of scope for this > proposal). > > > > > > > > I think it would be good to move this part into the "Future Work" > > section. > > > > > > > > > The Reason field is an optional textual description of why the > event > > is > > > > being sent > > > > > > > > Since we implemented optional fields in KIP-482, describing this > field > > as > > > > "optional" might be confusing. Probably better to avoid describing > it > > that > > > > way, unless it's a tagged field. > > > > > > > > > - If no Topic is given, it is implied that all topics on this > broker > > are > > > > being indicated > > > > > - If a Topic and no partitions are given, it is implied that all > > > > partitions of this topic are being indicated > > > > > > > > I would prefer to leave out these "shortcuts" since they seem likely > to > > > > lead to confusion and bugs. > > > > > > > > For example, suppose that the controller has just created a new > > partition > > > > for topic "foo" and put it on broker 3. But then, before broker 3 > > gets the > > > > LeaderAndIsrRequest from the controller, broker 3 get a bad log > > directory. > > > > So it sends an AlterReplicaStateRequest to the controller specifying > > topic > > > > foo and leaving out the partition list (using the first "shortcut".) > > The > > > > new partition will get marked as offline even though it hasn't even > > been > > > > created, much less assigned to the bad log directory. > > > > > > > > Since log directory failures are rare, spelling out the full set of > > > > affected partitions when one happens doesn't seem like that much of a > > > > burden. This is also consistent with what we currently do. In fact, > > it's > > > > much more efficient than what we currently do, since with KIP-589, we > > won't > > > > have to enumerate partitions that aren't on the failed log directory. > > > > > > > > In the future work section: If we eventually want to replace > > > > ControlledShutdownRequest with this RPC, we'll need some additional > > > > functionality. Specifically, we'll need the ability to tell the > > controller > > > > to stop putting new partitions on the broker that sent the request. > > That > > > > could be done with a separate request or possibly additional flags on > > this > > > > request. In any case, we don't have to solve that problem now. > > > > > > > > Thanks again for the KIP... great to see this moving forward. > > > > > > > > regards, > > > > Colin > > > > > > > > > > > > On Wed, May 20, 2020, at 12:22, David Arthur wrote: > > > > > Hello, all. I'd like to start the vote for KIP-589 which proposes > to > > add > > > > a > > > > > new AlterReplicaState RPC. > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-589+Add+API+to+update+Replica+state+in+Controller > > > > > > > > > > Cheers, > > > > > David > > > > > > > > > > > > > > > > > > -- > > > -David > > > > > > > > -- > -- Guozhang >
