On Fri, Oct 23, 2020, at 16:10, Jun Rao wrote: > Hi, Colin, > > Thanks for the reply. A few more comments.
Hi Jun, Thanks again for the reply. Sorry for the long hiatus. I was on vacation for a while. > > 55. There is still text that favors new broker registration. "When a broker > first starts up, when it is in the INITIAL state, it will always "win" > broker ID conflicts. However, once it is granted a lease, it transitions > out of the INITIAL state. Thereafter, it may lose subsequent conflicts if > its broker epoch is stale. (See KIP-380 for some background on broker > epoch.) The reason for favoring new processes is to accommodate the common > case where a process is killed with kill -9 and then restarted. We want it > to be able to reclaim its old ID quickly in this case." > Thanks for the reminder. I have clarified the language here. Hopefully now it is clear that we don't allow quick re-use of broker IDs. > 80.1 Sounds good. Could you document that listeners is a required config > now? It would also be useful to annotate other required configs. For > example, controller.connect should be required. > I added a note specifying that these are required. > 80.2 Could you list all deprecated existing configs? Another one is > control.plane.listener.name since the controller no longer sends > LeaderAndIsr, UpdateMetadata and StopReplica requests. > I added a section specifying some deprecated configs. > 83.1 It seems that the broker can transition from FENCED to RUNNING without > registering for a new broker epoch. I am not sure how this works. Once the > controller fences a broker, there is no need for the controller to keep the > boker epoch around. So, if the fenced broker's heartbeat request with the > existing broker epoch will be rejected, leading the broker back to the > FENCED state again. > The broker epoch refers to the broker registration. So we DO keep the broker epoch around even while the broker is fenced. The broker epoch changes only when there is a new broker registration. Fencing or unfencing the broker doesn't change the broker epoch. > 83.5 Good point on KIP-590. Then should we expose the controller for > debugging purposes? If not, we should deprecate the controllerID field in > MetadataResponse? > I think it's OK to expose it for now, with the proviso that it won't be reachable by clients. > 90. We rejected the shared ID with just one reason "This is not a good idea > because NetworkClient assumes a single ID space. So if there is both a > controller 1 and a broker 1, we don't have a way of picking the "right" > one." This doesn't seem to be a strong reason. For example, we could > address the NetworkClient issue with the node type as you pointed out or > using the negative value of a broker ID as the controller ID. > It would require a lot of code changes to support multiple types of node IDs. It's not clear to me that the end result would be better -- I tend to think it would be worse, since it would be more complex. In a similar vein, using negative numbers seems dangerous, since we use negatives or -1 as "special values" in many places. For example, -1 often represents "no such node." One important thing to keep in mind is that we want to be able to transition from a broker and a controller being co-located to them no longer being co-located. This is much easier to do when they have separate IDs. > 100. In KIP-589 > <https://cwiki.apache.org/confluence/display/KAFKA/KIP-589+Add+API+to+update+Replica+state+in+Controller>, > the broker reports all offline replicas due to a disk failure to the > controller. It seems this information needs to be persisted to the > metadata > log. Do we have a corresponding record for that? > Hmm, I have to look into this a little bit more. We may need a new record type. > 101. Currently, StopReplica request has 2 modes, without deletion and with > deletion. The former is used for controlled shutdown and handling disk > failure, and causes the follower to stop. The latter is for topic deletion > and partition reassignment, and causes the replica to be deleted. Since we > are deprecating StopReplica, could we document what triggers the stopping > of a follower and the deleting of a replica now? > RemoveTopic triggers deletion. In general the functionality of StopReplica is subsumed by the metadata records. > 102. Should we include the metadata topic in the MetadataResponse? If so, > when it will be included and what will the metadata response look like? > No, it won't be included in the metadata response sent back from the brokers. > 103. "The active controller assigns the broker a new broker epoch, based on > the latest committed offset in the log." This seems inaccurate since the > latest committed offset doesn't always advance on every log append. > Given that the new broker epoch won't be visible until the commit has happened, I have changed this to "the next available offset in the log" > 104. REGISTERING(1) : It says "Otherwise, the broker moves into the FENCED > state.". It seems this should be RUNNING? > > 105. RUNNING: Should we require the broker to catch up to the metadata log > to get into this state? For 104 and 105, these sections have been reworked. best, Colin > > Thanks, > > Jun > > > > On Fri, Oct 23, 2020 at 1:20 PM Colin McCabe <cmcc...@apache.org> wrote: > > > On Wed, Oct 21, 2020, at 05:51, Tom Bentley wrote: > > > Hi Colin, > > > > > > On Mon, Oct 19, 2020, at 08:59, Ron Dagostino wrote: > > > > > Hi Colin. Thanks for the hard work on this KIP. > > > > > > > > > > I have some questions about what happens to a broker when it becomes > > > > > fenced (e.g. because it can't send a heartbeat request to keep its > > > > > lease). The KIP says "When a broker is fenced, it cannot process any > > > > > client requests. This prevents brokers which are not receiving > > > > > metadata updates or that are not receiving and processing them fast > > > > > enough from causing issues to clients." And in the description of the > > > > > FENCED(4) state it likewise says "While in this state, the broker > > does > > > > > not respond to client requests." It makes sense that a fenced broker > > > > > should not accept producer requests -- I assume any such requests > > > > > would result in NotLeaderOrFollowerException. But what about KIP-392 > > > > > (fetch from follower) consumer requests? It is conceivable that > > these > > > > > could continue. Related to that, would a fenced broker continue to > > > > > fetch data for partitions where it thinks it is a follower? Even if > > > > > it rejects consumer requests it might still continue to fetch as a > > > > > follower. Might it be helpful to clarify both decisions here? > > > > > > > > Hi Ron, > > > > > > > > Good question. I think a fenced broker should continue to fetch on > > > > partitions it was already fetching before it was fenced, unless it > > hits a > > > > problem. At that point it won't be able to continue, since it doesn't > > have > > > > the new metadata. For example, it won't know about leadership changes > > in > > > > the partitions it's fetching. The rationale for continuing to fetch > > is to > > > > try to avoid disruptions as much as possible. > > > > > > > > I don't think fenced brokers should accept client requests. The issue > > is > > > > that the fenced broker may or may not have any data it is supposed to > > > > have. It may or may not have applied any configuration changes, etc. > > that > > > > it is supposed to have applied. So it could get pretty confusing, and > > also > > > > potentially waste the client's time. > > > > > > > > > > > When fenced, how would the broker reply to a client which did make a > > > request? > > > > > > > Hi Tom, > > > > The broker will respond with a retryable error in that case. Once the > > client has re-fetched its metadata, it will no longer see the fenced broker > > as part of the cluster. I added a note to the KIP. > > > > best, > > Colin > > > > > > > > Thanks, > > > > > > Tom > > > > > >
