Thanks, David. With these changes, I am +1 (binding) Great to see this KIP moving forward!
Colin On Wed, Jun 1, 2022, at 02:04, David Jacot wrote: > Hi Colin, > > Thanks for your feedback! Please find my answers below. > >> However, I wonder if this will be feasible for ZK-based brokers to do. We >> still support pre-topic-id IBP versions there, right? This might end up >> being more complex than you were hoping. > > That's right. We support pre-topic-id IBP versions for ZK-based > brokers. We will only use version 2 if all the topics have an ID when > the request is constructed. Version 1 is used otherwise. I have > already implemented this part in the draft PR if you want to see how > it looks. It brings a little more complexity in the request > building/handling but that works. I think that it is definitely worth > it. > >> We should add a comment in AlterPartitionResponse about the new error code >> INELIGIBLE_REPLICA. This is very important for error codes so we can track >> which ones are returned in which RPC version. > > Totally. I have those comments in the PR but I forgot to add them in > the KIP. I will fix this. > >> I also wonder if we should add a special error code for the case where the >> AlterPartitions call completes a reassignment AND the completed reassignment >> no longer includes the previous leader. We have been overloading >> FENCED_LEADER_EPOCH for this case, but this is coonfusing to operators >> (especially since this RPC does not support error messages, as opposed to >> codes) > > Interesting. I was not aware of this one. We don't do this in the ZK > controller, right? I do agree that using FENCED_LEADER_EPOCH is > confusing here. We could introduce a NEW_LEADER_ELECTED error code for > this purpose. > >> One thing, though, is that we should define how this interacts with replica >> placement. It seems to me that replicas should not be able to be placed on >> these inControlledShutdown nodes (unless done manually via the explicit >> placement API). > > Yeah, I agree that we need to clarify this. If not mistaken, replicas > can be placed on fenced nodes so I don't see why it should be > different for inControlledShutdown nodes. Otherwise we will again have > that create topic issue when the cluster has only 3 nodes. However I > think that we have to guarantee two other invariants: > 1) an inControlledShutdown node should not be added to the ISR when a > partition is created. > 2) an inControlledShutdown node should not be picked as a leader for a > partition. (e.g. KAFKA-13944) > > Let me add this part to the KIP. > >> I also think we should spell out the fact that once you go into controlled >> shutdown, you don't come out except by creating a new broker instance. (new >> incarnation ID). This also makes me wonder if we need to support the >> shutting down -> not shutting down transition in >> BrokerRegistrationChangeRecord, since we don't plan on using that transition. > > That makes sense. I will add this and remove that transition from > BrokerRegistrationChangeRecord. > >> Finally, RegisterBrokerRecord / BrokerRegistrationChangeRecord should be >> bumped to the next RPC version since we have added a new field. You will >> also need to assign yourself a new IBP / MetadataVersion. (For >> BrokerRegistrationChangeRecord it would be possible to avoid the version >> bump, since we're using tagged fields, but it's better to have it for >> consistency, I think.) > > Noted. > > Let me update the KIP to incorporate all your feedback. > > Cheers, > David > > On Tue, May 31, 2022 at 9:42 PM Colin McCabe <cmcc...@apache.org> wrote: >> >> > We should add a comment in AlterPartitionResponse about the new error >> > code INELIGIBLE_REPLICA. This is very important for error codes so we >> > can track which ones are returned in which RPC version. I also wonder >> >> Here I'm referring to AlterPartitionResponse.json >> >> cheers, >> Colin >> >> > >> > >> > On Tue, May 31, 2022, at 08:36, David Jacot wrote: >> >> Hi folks, >> >> >> >> I'd like to start a vote for KIP-841: >> >> https://cwiki.apache.org/confluence/x/phmhD. >> >> >> >> Thanks, >> >> David
