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
