On Wed, Dec 16, 2020, at 18:13, Jun Rao wrote: > Hi, Colin, > > Thanks for the reply. Just a couple of more comments. > > 211. Currently, the broker only registers itself in ZK after log recovery. > Is there any benefit to change that? As you mentioned, the broker can't do > much before completing log recovery. >
Hi Jun, Previously, it wasn't possible to register in ZK without immediately getting added to the MetadataResponse. So I think that's the main reason why registration was delayed until after log recovery. Since that constraint doesn't exist any more, there seems to be no reason to delay registration. I think delaying registration would have some major downsides. If log recovery takes a while, that's a longer window during which someone else could register a broker with the same ID. Having parts of the cluster missing for a while gives up some of the benefit of separating registration from fencing. For example, if a broker somehow gets unregistered and we want to re-register it, but we have to wait for a 10 minute log recovery to do that, that could be a window during which topics can't be created that need to be on that broker, and so forth. > 230. Regarding MetadataResponse, there is a slight awkwardness. We return > rack for each node. However, if that node is for the controller, the rack > field is not really relevant. Should we clean it up here or in another KIP > like KIP-700? Oh, controllers don't appear in the MetadataResponses returned to clients, since clients can't access them. I should have been more clear about that in the KIP-- I added a sentence to "Networking" describing this. best, Colin > > Thanks, > > Jun > > On Wed, Dec 16, 2020 at 4:23 PM Colin McCabe <cmcc...@apache.org> wrote: > > > On Wed, Dec 16, 2020, at 13:40, Jun Rao wrote: > > > Hi, Colin, > > > > > > Thanks for the reply. A few follow up comments. > > > > > > 211. When does the broker send the BrokerRegistration request to the > > > controller? Is it after the recovery phase? If so, at that point, the > > > broker has already caught up on the metadata (in order to clean up > > deleted > > > partitions). Then, it seems we don't need the ShouldFence field > > > in BrokerHeartbeatRequest? > > > > Hi Jun, > > > > Thanks again for the reviews. > > > > The broker sends the registration request as soon as it starts up. It > > cannot wait until the recovery phase is over since sometimes log recovery > > can take quite a long time. > > > > > > > > 213. kafka-cluster.sh > > > 213.1 For the decommision example, should the command take a broker id? > > > > Yes, the example should have a broker id. Fixed. > > > > > 213.2 Existing tools use the "--" command line option (e.g. kafka-topics > > > --list --topic test). Should we follow the same convention > > > for kafka-cluster.sh (and kafka-storage.sh)? > > > > Hmm. I don't think argparse4j supports using double dashes to identify > > subcommands. I think it might be confusing as well, since the subcommand > > must come first, unlike a plain old argument which can be anywhere on the > > command line. > > > > > 213.3 Should we add an admin api for broker decommission so that this can > > > be done programmatically? > > > > > > > Yes, there is an admin client API for decommissioning as well. > > > > > 220. DecommissionBroker: "NOT_CONTROLLER if the node that the request was > > > sent to is not the controller". I thought the clients never send a > > request > > > to the controller directly now and the broker will forward it to the > > > controller? > > > > > > > If the controller moved recently, it's possible that the broker could send > > to a controller that has just recently become inactive. In that case > > NOT_CONTROLLER would be returned. (A standby controller returns > > NOT_CONTROLLER for most APIs). > > > > > 221. Could we add the required ACL for the new requests? > > > > > > > Good point. I added the required ACL for each new RPC. > > > > best, > > Colin > > > > > > > Jun > > > > > > On Wed, Dec 16, 2020 at 11:38 AM Colin McCabe <cmcc...@apache.org> > > wrote: > > > > > > > On Wed, Dec 16, 2020, at 09:59, Jun Rao wrote: > > > > > Hi, Colin, > > > > > > > > > > Thanks for the reply. A few more comments below. > > > > > > > > > > > > > Hi Jun, > > > > > > > > Thanks for the comments. > > > > > > > > > 206. "RemoveTopic is the last step, that scrubs all metadata about > > the > > > > > topic. In order to get to that last step, the topic data needs to > > > > removed > > > > > from all brokers (after each broker notices that the topic is being > > > > > deleted)." Currently, this is done based on the response of > > > > > StopReplicaRequest. Since the controller no longer sends this > > request, > > > > how > > > > > does the controller know that the data for the deleted topic has > > > > > been removed in the brokers? > > > > > > > > > > > > > That's a good point... definitely an oversight. > > > > > > > > It seems complex to force the controller to track when log directories > > > > have been deleted. Let's just assume that KIP-516 has been > > implemented, > > > > and track them by UUID. Then we can just have a single topic deletion > > > > record. > > > > > > > > I added a section on "topic identifiers" describing this. > > > > > > > > > 210. Thanks for the explanation. Sounds good to me. > > > > > > > > > > 211. I still don't see an example when ShouldFence is set to true in > > > > > BrokerHeartbeatReques. Could we add one? > > > > > > > > > > > > > It is sent to true when the broker is first starting up and doesn't yet > > > > want to be unfenced. I added a longer explanation of this in the > > "Broker > > > > Leases" section. > > > > > > > > > 213. The KIP now allows replicas to be assigned on brokers that are > > > > fenced, > > > > > which is an improvement. How do we permanently remove a broker (e.g. > > > > > cluster shrinking) to prevent it from being used for future replica > > > > > assignments? > > > > > > > > > > > > > This is a fair point. I will create a kafka-cluster.sh script which > > can > > > > do this, plus a DecommissionBrokerRequest. > > > > > > > > As a bonus the kafka-cluster.sh script can help people find the > > cluster ID > > > > of brokers, something that people have wanted a tool for in the past. > > > > > > > > > 214. "Currently, when a node is down, all of its ZK registration > > > > > information is gone. But we need this information in order to > > > > understand > > > > > things like whether the replicas of a particular partition are > > > > > well-balanced across racks." This is not quite true. Currently, even > > when > > > > > ZK registration is gone, the existing replica assignment is still > > > > available > > > > > in the metadata response. > > > > > > > > > > > > > I agree that the existing replica assignment is still available. But > > that > > > > just tells you partition X is on nodes A, B, and C. If you don't have > > the > > > > ZK registration for one or more of A, B, or C then you don't know > > whether > > > > we are following the policy of "two replicas on one rack, one replica > > on > > > > another." Or any other more complex rack placement policy that you > > might > > > > have. > > > > > > > > best, > > > > Colin > > > > > > > > > Jun > > > > > > > > > > On Wed, Dec 16, 2020 at 8:48 AM Colin McCabe <cmcc...@apache.org> > > wrote: > > > > > > > > > > > On Tue, Dec 15, 2020, at 13:08, Jun Rao wrote: > > > > > > > Hi, Colin, > > > > > > > > > > > > > > Thanks for the reply. A few more follow up comments. > > > > > > > > > > > > > > 210. initial.broker.registration.timeout.ms: The default value > > is > > > > 90sec, > > > > > > > which seems long. If a broker fails the registration because of > > > > incorrect > > > > > > > configs, we want to fail faster. In comparison, the defaults for > > > > > > > zookeeper.connection.timeout.ms is 18 secs. > > > > > > > > > > > > > > > > > > > Hi Jun, > > > > > > > > > > > > I agree that the initial connection timeout here is longer than > > what we > > > > > > had with ZK. The main reason I selected a slightly longer timeout > > > > here is > > > > > > to handle the case where the controllers and the brokers are > > > > co-located. > > > > > > For example, if you have a 3 node cluster and all three nodes are > > > > > > controllers+brokers, when you first bring up the cluster, we will > > have > > > > to > > > > > > stand up the controller quorum and then handle broker > > registrations. > > > > > > Although we believe Raft will start up relatively quickly, it's > > good to > > > > > > leave some extra margin here. > > > > > > > > > > > > I don't think there's a big disadvantage to having a slightly > > longer > > > > > > timeout here. After all, starting up the brokers with no > > controllers > > > > is an > > > > > > admin mistake, which we don't expect to see very often. Maybe > > let's > > > > set it > > > > > > to 60 seconds for now and maybe see if we can tweak it in the > > future if > > > > > > that turns out to be too long or short? > > > > > > > > > > > > > 211. "Once they are all moved, the controller responds to the > > > > heartbeat > > > > > > > with a nextState of SHUTDOWN." It seems that nextState is no > > longer > > > > > > > relevant. Also, could you add a bit more explanation on when > > > > ShouldFence > > > > > > is > > > > > > > set to true in BrokerHeartbeatRequest? > > > > > > > > > > > > > > > > > > > Good catch. I removed the obsolete section referring to nextState > > and > > > > > > added a reference to the new boolean. I also added some more > > > > information > > > > > > about ShouldFence and about the rationale for separating fencing > > from > > > > > > registration. > > > > > > > > > > > > > 212. Related to the above, what does the broker do if IsFenced is > > > > true in > > > > > > > the BrokerHeartbeatResponse? Will the broker do the same thing on > > > > > > receiving > > > > > > > a FenceBrokerRecord from the metadata log? > > > > > > > > > > > > > > > > > > > The broker only checks this boolean during the startup process. > > After > > > > the > > > > > > startup process is finished, it ignores this boolean. > > > > > > > > > > > > The broker uses fence / unfence records in the metadata log to > > > > determine > > > > > > which brokers should appear in its MetadataResponse. > > > > > > > > > > > > best, > > > > > > Colin > > > > > > > > > > > > > Jun > > > > > > > > > > > > > > On Tue, Dec 15, 2020 at 8:51 AM Colin McCabe <cmcc...@apache.org > > > > > > > wrote: > > > > > > > > > > > > > > > On Tue, Dec 15, 2020, at 04:13, Tom Bentley wrote: > > > > > > > > > Hi Colin, > > > > > > > > > > > > > > > > > > The KIP says that "brokers which are fenced will not appear > > in > > > > > > > > > MetadataResponses. So clients that have up-to-date metadata > > will > > > > > > not try > > > > > > > > > to contact fenced brokers.", which is fine, but it doesn't > > seem > > > > to > > > > > > > > > elaborate on what happens for clients which try to contact a > > > > broker > > > > > > > > (using > > > > > > > > > stale metadata, for example) and find it in the > > > > registered-but-fenced > > > > > > > > > state. > > > > > > > > > > > > > > > > Hi Tom, > > > > > > > > > > > > > > > > I have dropped the broker-side fencing from this proposal. So > > now > > > > the > > > > > > > > fencing is basically the same as today's fencing: it's > > > > controller-side > > > > > > > > only. That means that clients using stale metadata can contact > > > > fenced > > > > > > > > brokers and communicate with them. The only case where the > > broker > > > > > > will be > > > > > > > > inaccessible is when it hasn't finished starting yet (i.e., has > > > > not yet > > > > > > > > attained RUNNING state.) > > > > > > > > > > > > > > > > Just like today, we expect the safeguards built into the > > > > replication > > > > > > > > protocol to prevent the worst corner cases that could result > > from > > > > > > this. I > > > > > > > > do think we will probably take up this issue later and find a > > > > better > > > > > > > > solution for client-side metadata, but this KIP is big enough > > > > as-is. > > > > > > > > > > > > > > > > best, > > > > > > > > Colin > > > > > > > > > > > > > > > > > > > > > > > > > You said previously that the broker will respond with a > > retriable > > > > > > > > > error, which is again fine, but you didn't answer my question > > > > about > > > > > > which > > > > > > > > > error code you've chosen for this. I realise that this > > doesn't > > > > really > > > > > > > > > affect the correctness of the behaviour, but I'm not aware > > of any > > > > > > > > existing > > > > > > > > > error code which is a good fit. So it would be good to > > understand > > > > > > about > > > > > > > > how > > > > > > > > > you're making this backward compatible for clients. > > > > > > > > > > > > > > > > > > Many thanks, > > > > > > > > > > > > > > > > > > Tom > > > > > > > > > > > > > > > > > > On Tue, Dec 15, 2020 at 1:42 AM Colin McCabe < > > cmcc...@apache.org > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > On Fri, Dec 11, 2020, at 17:07, Jun Rao wrote: > > > > > > > > > > > Hi, Colin, > > > > > > > > > > > > > > > > > > > > > > Thanks for the reply. Just a couple of more comments > > below. > > > > > > > > > > > > > > > > > > > > > > 210. Since we are deprecating > > > > zookeeper.connection.timeout.ms, > > > > > > > > should we > > > > > > > > > > > add a new config to bound the time for a broker to > > connect > > > > to the > > > > > > > > > > > controller during starting up? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Good idea. I added initial.broker.registration.timeout.ms > > for > > > > > > this. > > > > > > > > > > > > > > > > > > > > > 211. BrokerHeartbeat no longer has the state field in the > > > > > > > > > > request/response. > > > > > > > > > > > However, (a) the controller shutdown section still has > > "In > > > > its > > > > > > > > periodic > > > > > > > > > > > heartbeats, the broker asks the controller if it can > > > > transition > > > > > > into > > > > > > > > the > > > > > > > > > > > SHUTDOWN state. This motivates the controller to move > > all > > > > of the > > > > > > > > leaders > > > > > > > > > > > off of that broker. Once they are all moved, the > > controller > > > > > > > > responds to > > > > > > > > > > > the heartbeat with a nextState of SHUTDOWN."; (2) the > > > > > > description of > > > > > > > > > > > BrokerHeartbeat still references currentState and > > > > targetState. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks. I've made these sections clearer and removed the > > > > obsolete > > > > > > > > > > references to sending states. > > > > > > > > > > > > > > > > > > > > best, > > > > > > > > > > Colin > > > > > > > > > > > > > > > > > > > > > Jun > > > > > > > > > > > > > > > > > > > > > > On Fri, Dec 11, 2020 at 1:33 PM Colin McCabe < > > > > cmcc...@apache.org > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > On Wed, Dec 9, 2020, at 10:10, Jun Rao wrote: > > > > > > > > > > > > > Hi, Colin, > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks for the update. A few more follow up comments. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Jun, > > > > > > > > > > > > > > > > > > > > > > > > Thanks again for the review. > > > > > > > > > > > > > > > > > > > > > > > > > 100. FailedReplicaRecord: Since this is reported by > > each > > > > > > broker > > > > > > > > > > > > > independently, perhaps we could use a more concise > > > > > > representation > > > > > > > > > > that > > > > > > > > > > > > has > > > > > > > > > > > > > a top level broker field, an array of topics, which > > has > > > > an > > > > > > array > > > > > > > > of > > > > > > > > > > > > > partitions. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The issue is that there is a size limit on the each > > record. > > > > > > > > Putting > > > > > > > > > > all > > > > > > > > > > > > of the partitions of a log directory into a single > > record > > > > would > > > > > > > > > > probably > > > > > > > > > > > > break that in many cases. Still, we can optimize a > > bit by > > > > > > having > > > > > > > > an > > > > > > > > > > array > > > > > > > > > > > > of partition IDs, since nearly all the time, we have > > more > > > > than > > > > > > one > > > > > > > > > > from the > > > > > > > > > > > > same topic. > > > > > > > > > > > > > > > > > > > > > > > > > 200. Sounds good. If we remove the broker-side > > fencing > > > > > > logic, do > > > > > > > > we > > > > > > > > > > plan > > > > > > > > > > > > to > > > > > > > > > > > > > still keep FENCED in broker state? Do we plan to > > expose > > > > the > > > > > > new > > > > > > > > > > states > > > > > > > > > > > > > through the existing BrokerState metric and if so, > > what > > > > are > > > > > > the > > > > > > > > > > values > > > > > > > > > > > > for > > > > > > > > > > > > > the new states? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > No, we don't need FENCED any more. I have removed it > > from > > > > the > > > > > > KIP. > > > > > > > > > > > > > > > > > > > > > > > > The new states are very similar to the current ones, > > > > actually. > > > > > > > > There > > > > > > > > > > are > > > > > > > > > > > > no new states or removed ones. The main change in the > > > > broker > > > > > > state > > > > > > > > > > machine > > > > > > > > > > > > is that the RECOVERING_FROM_UNCLEAN_SHUTDOWN state has > > been > > > > > > > > renamed to > > > > > > > > > > > > RECOVERY. Also, unlike previously, the broker will > > always > > > > pass > > > > > > > > through > > > > > > > > > > > > RECOVERY (although it may only stay in this state for a > > > > very > > > > > > short > > > > > > > > > > amount > > > > > > > > > > > > of time). > > > > > > > > > > > > > > > > > > > > > > > > > 201. This may be fine too. Could we document what > > happens > > > > > > when > > > > > > > > the > > > > > > > > > > > > > broker.id/controller.id in metadata.properties don't > > > > match > > > > > > the > > > > > > > > > > broker > > > > > > > > > > > > > config when the broker starts up? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I added some documentation about this. > > > > > > > > > > > > > > > > > > > > > > > > > 204. There is still "The highest metadata offset > > which > > > > the > > > > > > broker > > > > > > > > > > has not > > > > > > > > > > > > > reached" referenced under BrokerRegistration. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > It should be CurrentMetadataOffset. Fixed. > > > > > > > > > > > > > > > > > > > > > > > > > 206. Is that separate step needed given KIP-516? With > > > > > > KIP-516 ( > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-516%3A+Topic+Identifiers#KIP516:TopicIdentifiers-LeaderAndIsr > > > > > > > > > > > > ), > > > > > > > > > > > > > we don't need to wait for the topic data to be > > removed > > > > from > > > > > > all > > > > > > > > > > brokers > > > > > > > > > > > > > before removing the topic metadata. The combination > > of > > > > > > unmatching > > > > > > > > > > > > > topicId > > > > > > > > > > > > > or the missing topicId from the metadata is enough > > for > > > > the > > > > > > > > broker to > > > > > > > > > > > > > clean > > > > > > > > > > > > > up deleted topics asynchronously. > > > > > > > > > > > > > > > > > > > > > > > > It won't be needed once KIP-516 is adopted, but this > > hasn't > > > > > > been > > > > > > > > > > > > implemented yet. > > > > > > > > > > > > > > > > > > > > > > > > best, > > > > > > > > > > > > Colin > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Jun > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Dec 8, 2020 at 5:27 PM Colin McCabe < > > > > > > cmcc...@apache.org> > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Dec 3, 2020, at 16:37, Jun Rao wrote: > > > > > > > > > > > > > > > Hi, Colin, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks for the updated KIP. A few more comments > > > > below. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Jun, > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks again for the reviews. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 80.2 For deprecated configs, we need to include > > > > > > zookeeper.* > > > > > > > > and > > > > > > > > > > > > > > > broker.id.generation.enable. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Added. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 83.1 If a broker is down, does the controller > > keep > > > > the > > > > > > > > previously > > > > > > > > > > > > > > > registered broker epoch forever? If not, how long > > > > does > > > > > > the > > > > > > > > > > controller > > > > > > > > > > > > > > keep > > > > > > > > > > > > > > > it? What does the controller do when receiving a > > > > broker > > > > > > > > heartbeat > > > > > > > > > > > > request > > > > > > > > > > > > > > > with an unfound broker epoch? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Yes, the controller keeps the previous registration > > > > > > forever. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Broker heartbeat requests with an incorrect broker > > > > epoch > > > > > > will > > > > > > > > be > > > > > > > > > > > > rejected > > > > > > > > > > > > > > with STALE_BROKER_EPOCH. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 100. Have you figured out if we need to add a new > > > > record > > > > > > > > type for > > > > > > > > > > > > > > reporting > > > > > > > > > > > > > > > partitions on failed disks? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I added FailedReplicaRecord to reflect the case > > where a > > > > > > JBOD > > > > > > > > > > directory > > > > > > > > > > > > has > > > > > > > > > > > > > > failed, leading to failed replicas. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 102. For debugging purposes, sometimes it's > > useful to > > > > > > read > > > > > > > > the > > > > > > > > > > > > metadata > > > > > > > > > > > > > > > topic using tools like console-consumer. Should > > we > > > > > > support > > > > > > > > that > > > > > > > > > > and > > > > > > > > > > > > if > > > > > > > > > > > > > > so, > > > > > > > > > > > > > > > how? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > For now, we have the ability to read the metadata > > logs > > > > > > with the > > > > > > > > > > > > dump-logs > > > > > > > > > > > > > > tool. I think we will come up with some other > > tools > > > > in the > > > > > > > > future > > > > > > > > > > as > > > > > > > > > > > > we > > > > > > > > > > > > > > get experience. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 200. "brokers which are fenced will not appear in > > > > > > > > > > MetadataResponses. > > > > > > > > > > > > The > > > > > > > > > > > > > > > broker will not respond to these requests-- > > instead, > > > > it > > > > > > will > > > > > > > > > > simply > > > > > > > > > > > > > > > disconnect." If the controller is partitioned off > > > > from > > > > > > the > > > > > > > > > > brokers, > > > > > > > > > > > > this > > > > > > > > > > > > > > > design will cause every broker to stop accepting > > new > > > > > > client > > > > > > > > > > > > requests. In > > > > > > > > > > > > > > > contrast, if ZK is partitioned off, the existing > > > > > > behavior is > > > > > > > > > > that the > > > > > > > > > > > > > > > brokers can continue to work based on the last > > known > > > > > > > > metadata. > > > > > > > > > > So, I > > > > > > > > > > > > am > > > > > > > > > > > > > > not > > > > > > > > > > > > > > > sure if we should change the existing behavior > > > > because > > > > > > of the > > > > > > > > > > bigger > > > > > > > > > > > > > > impact > > > > > > > > > > > > > > > in the new one. Another option is to keep the > > > > existing > > > > > > > > behavior > > > > > > > > > > and > > > > > > > > > > > > > > expose > > > > > > > > > > > > > > > a metric for fenced brokers so that the operator > > > > could be > > > > > > > > > > alerted. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I'm skeptical about how well running without ZK > > > > currently > > > > > > > > works. > > > > > > > > > > > > However, > > > > > > > > > > > > > > I will move the broker-side fencing into a > > follow-up > > > > KIP. > > > > > > This > > > > > > > > > > KIP is > > > > > > > > > > > > > > already pretty large and there is no hard > > dependency on > > > > > > this. > > > > > > > > > > There > > > > > > > > > > > > may > > > > > > > > > > > > > > also be other ways of accomplishing the positive > > > > effects of > > > > > > > > what > > > > > > > > > > > > > > broker-side fencing, so more discussion is needed. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 201. I read Ron's comment, but I am still not > > sure > > > > the > > > > > > > > benefit of > > > > > > > > > > > > keeping > > > > > > > > > > > > > > > broker.id and controller.id in meta.properties. > > It > > > > seems > > > > > > > > that > > > > > > > > > > we are > > > > > > > > > > > > > > just > > > > > > > > > > > > > > > duplicating the same info in two places and have > > the > > > > > > > > additional > > > > > > > > > > > > burden of > > > > > > > > > > > > > > > making sure the values in the two places are > > > > consistent. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I think the reasoning is that having broker.id > > > > protects us > > > > > > > > against > > > > > > > > > > > > > > accidentally bringing up a broker with a disk from > > a > > > > > > different > > > > > > > > > > > > broker. I > > > > > > > > > > > > > > don't feel strongly about this but it seemed > > simpler to > > > > > > keep > > > > > > > > it. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 202. controller.connect.security.protocol: Is > > this > > > > needed > > > > > > > > since > > > > > > > > > > > > > > > controller.listener.names and > > > > > > listener.security.protocol.map > > > > > > > > > > imply > > > > > > > > > > > > the > > > > > > > > > > > > > > > security protocol already? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > You're right, this isn't needed. I'll remove it. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 203. registration.heartbeat.interval.ms: It > > > > defaults to > > > > > > 2k. > > > > > > > > ZK > > > > > > > > > > uses > > > > > > > > > > > > 1/3 > > > > > > > > > > > > > > of > > > > > > > > > > > > > > > the session timeout for heartbeat. So, given the > > > > default > > > > > > 18k > > > > > > > > for > > > > > > > > > > > > > > > registration.lease.timeout.ms, should we default > > > > > > > > > > > > > > > registration.heartbeat.interval.ms to 6k? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 6 seconds seems like a pretty long time between > > > > > > heartbeats. It > > > > > > > > > > might > > > > > > > > > > > > be > > > > > > > > > > > > > > useful to know when a broker is missing heartbeats, > > > > with > > > > > > less > > > > > > > > time > > > > > > > > > > than > > > > > > > > > > > > > > that. I provisionally set it to 3 seconds (we can > > > > always > > > > > > > > change > > > > > > > > > > > > later...) > > > > > > > > > > > > > > > > > > > > > > > > > > > > I also changed the name of these configurations to > > " > > > > > > > > > > > > > > broker.heartbeat.interval.ms" and " > > > > > > > > broker.registration.timeout.ms" > > > > > > > > > > to > > > > > > > > > > > > try > > > > > > > > > > > > > > to clarify them a bit. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 204. "The highest metadata offset which the > > broker > > > > has > > > > > > not > > > > > > > > > > reached." > > > > > > > > > > > > It > > > > > > > > > > > > > > > seems this should be "has reached". > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I changed this to "one more than the highest > > metadata > > > > > > offset > > > > > > > > which > > > > > > > > > > the > > > > > > > > > > > > > > broker has reached." > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 205. UnfenceBrokerRecord and > > UnregisterBrokerRecord: > > > > To > > > > > > me, > > > > > > > > they > > > > > > > > > > > > seem to > > > > > > > > > > > > > > be > > > > > > > > > > > > > > > the same. Do we need both? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Unregistration means that the broker has been > > removed > > > > from > > > > > > the > > > > > > > > > > cluster. > > > > > > > > > > > > > > That is different than unfencing, which marks the > > > > broker as > > > > > > > > active. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 206. TopicRecord: The Deleting field is used to > > > > indicate > > > > > > > > that the > > > > > > > > > > > > topic > > > > > > > > > > > > > > is > > > > > > > > > > > > > > > being deleted. I am wondering if this is really > > > > needed > > > > > > since > > > > > > > > > > > > RemoveTopic > > > > > > > > > > > > > > > already indicates the same thing. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > RemoveTopic is the last step, that scrubs all > > metadata > > > > > > about > > > > > > > > the > > > > > > > > > > topic. > > > > > > > > > > > > > > In order to get to that last step, the topic data > > > > needs to > > > > > > > > removed > > > > > > > > > > > > from all > > > > > > > > > > > > > > brokers (after each broker notices that the topic > > is > > > > being > > > > > > > > > > deleted). > > > > > > > > > > > > > > > > > > > > > > > > > > > > best, > > > > > > > > > > > > > > Colin > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Jun > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Dec 2, 2020 at 2:50 PM Colin McCabe < > > > > > > > > cmcc...@apache.org> > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Dec 2, 2020, at 14:07, Ron Dagostino > > wrote: > > > > > > > > > > > > > > > > > Hi Colin. Thanks for the updates. It's now > > > > clear > > > > > > to me > > > > > > > > that > > > > > > > > > > > > brokers > > > > > > > > > > > > > > > > > keep their broker epoch for the life of their > > > > JVM -- > > > > > > they > > > > > > > > > > > > register > > > > > > > > > > > > > > > > > once, get their broker epoch in the > > response, and > > > > > > then > > > > > > > > never > > > > > > > > > > > > > > > > > re-register again. Brokers may get fenced, > > but > > > > they > > > > > > > > keep the > > > > > > > > > > > > same > > > > > > > > > > > > > > > > > broker epoch for the life of their JVM. The > > > > > > incarnation > > > > > > > > ID > > > > > > > > > > is > > > > > > > > > > > > also > > > > > > > > > > > > > > > > > kept for the life of the JVM but is > > generated by > > > > the > > > > > > > > broker > > > > > > > > > > > > itself > > > > > > > > > > > > > > > > > upon startup, and the combination of the two > > > > allows > > > > > > the > > > > > > > > > > > > Controller to > > > > > > > > > > > > > > > > > act idempotently if any previously-sent > > > > registration > > > > > > > > response > > > > > > > > > > > > gets > > > > > > > > > > > > > > > > > lost. Makes sense. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, Ron. That's a good summary. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > One thing I wonder about is if it might be > > > > helpful > > > > > > for > > > > > > > > the > > > > > > > > > > > > broker to > > > > > > > > > > > > > > > > > send the Cluster ID as determined from its > > > > > > > > meta.properties > > > > > > > > > > file > > > > > > > > > > > > in > > > > > > > > > > > > > > its > > > > > > > > > > > > > > > > > registration request. Does it even make > > sense > > > > for > > > > > > the > > > > > > > > > > broker to > > > > > > > > > > > > > > > > > successfully register and enter the Fenced > > state > > > > if > > > > > > it > > > > > > > > has > > > > > > > > > > the > > > > > > > > > > > > wrong > > > > > > > > > > > > > > > > > Cluster ID? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Yeah, that's a good idea. Let's have the > > broker > > > > pass > > > > > > its > > > > > > > > > > cluster > > > > > > > > > > > > ID in > > > > > > > > > > > > > > > > the registration RPC, and then registration can > > > > fail > > > > > > if the > > > > > > > > > > broker > > > > > > > > > > > > is > > > > > > > > > > > > > > > > configured for the wrong cluster. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The nextMetadatOffset value that the broker > > > > > > communicates > > > > > > > > > > > > > > > > > in its registration request only has meaning > > > > within > > > > > > the > > > > > > > > > > correct > > > > > > > > > > > > > > > > > cluster, so it feels to me that the > > Controller > > > > should > > > > > > > > have > > > > > > > > > > some > > > > > > > > > > > > way > > > > > > > > > > > > > > to > > > > > > > > > > > > > > > > > perform this sanity check. There is > > currently > > > > > > (pre-KIP > > > > > > > > 500) > > > > > > > > > > a > > > > > > > > > > > > check > > > > > > > > > > > > > > > > > in the broker to make sure its configured > > > > cluster ID > > > > > > > > matches > > > > > > > > > > the > > > > > > > > > > > > one > > > > > > > > > > > > > > > > > stored in ZooKeeper, and we will have to > > perform > > > > this > > > > > > > > > > validation > > > > > > > > > > > > > > > > > somewhere in the KIP-500 world. If the > > > > Controller > > > > > > > > doesn't > > > > > > > > > > do it > > > > > > > > > > > > > > > > > within the registration request then the > > broker > > > > will > > > > > > > > have to > > > > > > > > > > > > make a > > > > > > > > > > > > > > > > > metadata request to the Controller, retrieve > > the > > > > > > Cluster > > > > > > > > ID, > > > > > > > > > > and > > > > > > > > > > > > > > > > > perform the check itself. It feels to me > > that it > > > > > > might > > > > > > > > be > > > > > > > > > > > > better for > > > > > > > > > > > > > > > > > the Controller to just do it, and then the > > broker > > > > > > doesn't > > > > > > > > > > have to > > > > > > > > > > > > > > > > > worry about it anymore once it successfully > > > > > > registers. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I also have a question about the broker.id > > > > value and > > > > > > > > > > > > > > meta.properties. > > > > > > > > > > > > > > > > > The KIP now says "In version 0 of > > > > meta.properties, > > > > > > there > > > > > > > > is a > > > > > > > > > > > > > > > > > broker.id field. Version 1 does not have > > this > > > > > > field. > > > > > > > > It > > > > > > > > > > is no > > > > > > > > > > > > > > longer > > > > > > > > > > > > > > > > > needed because we no longer support dynamic > > > > broker id > > > > > > > > > > > > assignment." > > > > > > > > > > > > > > > > > But then there is an example version 1 > > > > > > meta.properties > > > > > > > > file > > > > > > > > > > that > > > > > > > > > > > > > > shows > > > > > > > > > > > > > > > > > the broker.id value. I actually wonder if > > > > maybe the > > > > > > > > > > broker.id > > > > > > > > > > > > value > > > > > > > > > > > > > > > > > would be good to keep in the version 1 > > > > > > meta.properties > > > > > > > > file > > > > > > > > > > > > because > > > > > > > > > > > > > > it > > > > > > > > > > > > > > > > > currently (pre-KIP 500, version 0) acts as a > > > > sanity > > > > > > > > check to > > > > > > > > > > make > > > > > > > > > > > > > > sure > > > > > > > > > > > > > > > > > the broker is using the correct log > > directory. > > > > > > Similarly > > > > > > > > > > with > > > > > > > > > > > > the > > > > > > > > > > > > > > > > > controller.id value on controllers -- it > > would > > > > > > allow the > > > > > > > > > > same > > > > > > > > > > > > type > > > > > > > > > > > > > > of > > > > > > > > > > > > > > > > > sanity check for quorum controllers. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > That's a good point. I will add broker.id > > back, > > > > and > > > > > > also > > > > > > > > add > > > > > > > > > > > > > > > > controller.id as a possibility. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > cheers, > > > > > > > > > > > > > > > > Colin > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Nov 30, 2020 at 7:41 PM Colin McCabe > > < > > > > > > > > > > cmcc...@apache.org > > > > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
