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 >
