Hi Becket, Dong, Thanks for the discussion. Those are very good points. I think it makes sense to send back STALE_BROKER_EPOCH error to the broker in both cases:
1. Broker gets quickly restarted. In this case, the channel has already been closed during broker shutdown so the broker will not react to the error. 2. Broker gets disconnected from zk and reconnect. In this case, the broker will see the error and will resend the ControlledShutdownRequest with a newer broker epoch. I have also updated the KIP accordingly to include what we have discussed. Thanks again! Best, Zhanxiang (Patrick) Huang ________________________________ From: Becket Qin <becket....@gmail.com> Sent: Monday, November 12, 2018 21:56 To: Dong Lin Cc: dev Subject: Re: [DISCUSS] KIP-380: Detect outdated control requests and bounced brokers using broker generation Hi Dong, That is a good point. But I think the STALE_BROKER_EPOCH error may still be sent to the broker. For example, think about the following case: 1. Broker sends a ControlledShutdownRequest to the controller 2. Broker had a ZK session timeout 3. Broker created the ephemeral node 4. Controller processes the ControlledShutdownRequest in step 1 5. Broker receives a ControlledShutdownResponse with STALE_BROKER_EPOCH. However, in this case, the broker should probably resend the controlled shutdown request again with the new epoch. So it looks that returning a STALE_BROKER_EPOCH is the correct behavior. If the broker has really been bounced, that response will not be delivered to the broker. If the broker has not really restarted, it will just resend the ControlledShutdownRequest with the current epoch again. It might worth updating the KIP wiki to mention this behavior. Thanks, Jiangjie (Becket) Qin On Tue, Nov 13, 2018 at 2:04 AM Dong Lin <lindon...@gmail.com> wrote: > Hey Becket, Patrick, > > Currently we expect that controller can only receive ControlledShutdownRequest > with outdated broker epoch in two cases: 1) the ControlledShutdownRequest > was sent by the broker before it has restarted; and 2) there is bug. > > In case 1), it seems that ControlledShutdownResponse will not be delivered > to the broker as the channel should have been disconnected. Thus there is > no confusion to the broker because broker will not receive response > with STALE_BROKER_EPOCH error. > > In case 2), it seems that it can be useful to still deliver > ControlledShutdownResponse > with STALE_BROKER_EPOCH so that this broker at least knows that which > response is not accepted. This helps us debug this issue. > > Also, in terms of both design and implementation, it seems simpler to > still define a response for a given invalid request rather than have > special path to skip response for the invalid request. Does this sound > reasonable? > > Thanks, > Dong > > > On Mon, Nov 12, 2018 at 9:52 AM Patrick Huang <hzx...@hotmail.com> wrote: > >> Hi Becket, >> >> I think you are right. STALE_BROKER_EPOCH only makes sense when the >> broker detects outdated control requests and wants the controller to know >> about that. For ControlledShutdownRequest, the controller should just >> ignore the request with stale broker epoch since the broker does not need >> and will not do anything for STALE_BROKER_EPOCH response. Thanks for >> pointing it out. >> >> Thanks, >> Zhanxiang (Patrick) Huang >> >> ________________________________ >> From: Becket Qin <becket....@gmail.com> >> Sent: Monday, November 12, 2018 6:46 >> To: dev >> Subject: Re: [DISCUSS] KIP-380: Detect outdated control requests and >> bounced brokers using broker generation >> >> Hi Patrick, >> >> I am wondering why the controller should send STALE_BROKER_EPOCH error to >> the broker if the broker epoch is stale? Would this be a little confusing >> to the current broker if the request was sent by a broker with previous >> epoch? Should the controller just ignore those requests in that case? >> >> Thanks, >> >> Jiangjie (Becket) Qin >> >> On Fri, Nov 9, 2018 at 2:17 AM Patrick Huang <hzx...@hotmail.com> wrote: >> >> > Hi, >> > >> > In this KIP, we are also going to add a new exception and a new error >> code >> > "STALE_BROKER_EPOCH" in order to allow the broker to respond back the >> right >> > error when it sees outdated broker epoch in the control requests. Since >> > adding a new exception and error code is also considered as public >> > interface change, I have updated the original KIP accordingly to include >> > this change. Feel free to comment if there is any concern. >> > >> > Thanks, >> > Zhanxiang (Patrick) Huang >> > >> > ________________________________ >> > From: Patrick Huang <hzx...@hotmail.com> >> > Sent: Tuesday, October 23, 2018 6:20 >> > To: Jun Rao; dev@kafka.apache.org >> > Subject: Re: [DISCUSS] KIP-380: Detect outdated control requests and >> > bounced brokers using broker generation >> > >> > Agreed. I have updated the PR to add czxid in ControlledShutdownRequest >> ( >> > https://github.com/apache/kafka/pull/5821). Appreciated if you can >> take a >> > look. >> > >> > Btw, I also have the vote thread for this KIP: >> > >> https://lists.apache.org/thread.html/3689d83db537d5aa86d10967dee7ee29578897fc123daae4f77a8605@%3Cdev.kafka.apache.org%3E >> > >> > Best, >> > Zhanxiang (Patrick) Huang >> > >> > ________________________________ >> > From: Jun Rao <j...@confluent.io> >> > Sent: Monday, October 22, 2018 21:31 >> > To: dev@kafka.apache.org >> > Subject: Re: [DISCUSS] KIP-380: Detect outdated control requests and >> > bounced brokers using broker generation >> > >> > Hi, Patrick, >> > >> > Yes, that's the general sequence. After step 2, the shutting down broker >> > can give up the controlled shutdown process and proceed to shut down. >> When >> > it's restarted, it could still receive StopReplica requests from the >> > controller in reaction to the previous controlled shutdown requests. >> This >> > could lead the restarted broker to a bad state. >> > >> > Thanks, >> > >> > Jun >> > >> > >> > On Mon, Oct 22, 2018 at 4:32 PM, Patrick Huang <hzx...@hotmail.com> >> wrote: >> > >> > > Hi Jun, >> > > >> > > That is a good point. I want to make it clear about the scenario you >> > > mentioned. Correct me if I am wrong. Here is the sequence of the >> event: >> > > >> > > 1. Broker sends ControlledShutdown request 1 to controller >> > > 2. Broker sends ControlledShutdown request 2 to controller due to >> > > reties >> > > 3. Controller processes ControlledShutdown request 1 >> > > 4. Controller sends control requests to the broker >> > > 5. Broker receives ControlledShutdown response 1 from controller >> > > 6. Broker shuts down and restarts quickly >> > > 7. Controller processes ControllerShutdown request 2 >> > > 8. Controller sends control requests to the broker >> > > >> > > It is possible that controller processes the broker change event >> between >> > > 6) and 7). In this case, controller already updates the cached czxid >> to >> > the >> > > up-to-date ones so the bounced broker will not reject control >> requests in >> > > 8), which cause a correctness problem. >> > > >> > > >> > > Best, >> > > Zhanxiang (Patrick) Huang >> > > >> > > ------------------------------ >> > > *From:* Jun Rao <j...@confluent.io> >> > > *Sent:* Monday, October 22, 2018 14:45 >> > > *To:* dev >> > > *Subject:* Re: [DISCUSS] KIP-380: Detect outdated control requests and >> > > bounced brokers using broker generation >> > > >> > > Hi, Patrick, >> > > >> > > There is another thing that may be worth considering. >> > > >> > > 10. It will be useful to include the czxid also in the >> ControlledShutdown >> > > request. This way, if the broker has been restarted, the controller >> can >> > > ignore an old ControlledShutdown request(e.g., due to retries). This >> will >> > > prevent the restarted broker from incorrectly stopping replicas. >> > > >> > > Thanks, >> > > >> > > Jun >> > > >> > > >> > > On Thu, Oct 11, 2018 at 5:56 PM, Patrick Huang < >> hzxa21.hu...@gmail.com> >> > > wrote: >> > > >> > > > Hi Jun, >> > > > >> > > > Thanks a lot for the comments. >> > > > >> > > > 1. czxid is globally unique and monotonically increasing based on >> the >> > > > zookeeper doc. >> > > > References (from >> > > > https://zookeeper.apache.org/doc/r3.1.2/zookeeperProgrammers.html): >> > > > "Every change to the ZooKeeper state receives a stamp in the form >> of a >> > > > *zxid* (ZooKeeper Transaction Id). This exposes the total ordering >> of >> > all >> > > > changes to ZooKeeper. Each change will have a unique zxid and if >> zxid1 >> > is >> > > > smaller than zxid2 then zxid1 happened before zxid2." >> > > > "czxid: The zxid of the change that caused this znode to be >> created." >> > > > >> > > > 2. You are right. There will be only on broker change event fired in >> > the >> > > > case I mentioned because we will not register the watcher before the >> > > read. >> > > > >> > > > 3. Let's say we want to initialize the states of broker set A and we >> > want >> > > > the cluster to be aware of the absence of broker set B. The >> currently >> > > live >> > > > broker set in the cluster is C. >> > > > >> > > > From the design point of view, here are the rules for broker >> state >> > > > transition: >> > > > - Pass in broker ids of A for onBrokerStartup() and pass in >> broker >> > > ids >> > > > of B for onBrokerFailure(). >> > > > - When processing onBrokerStartup(), we use the broker >> generation >> > > > controller read from zk to send requests to broker set A and use the >> > > > previously cached broker generation to send requests to (C-A). >> > > > - When processing onBrokerFailure(), we use the previously >> cached >> > > > broker generation to send requests to C. >> > > > >> > > > From the implementation point of view, here are the steps we >> need >> > to >> > > > follow when processing BrokerChangeEvent: >> > > > - Reads all child nodes in /brokers/ids/ to get current brokers >> > with >> > > > broker generation >> > > > - Detect new brokers, dead brokers and bounced brokers >> > > > - Update the live broker ids in controller context >> > > > - Update broker generations for new brokers in controller >> context >> > > > - Invoke onBrokerStartup(new brokers) >> > > > - Invoke onBrokerFailure(bounced brokers) >> > > > - Update broker generations for bounce brokers in controller >> > context >> > > > - Invoke onBrokerStartup(bounced brokers) >> > > > - Invoke onBrokerFailure(dead brokers) >> > > > We can further optimize the flow by avoiding sending requests >> to a >> > > > broker if its broker generation is larger than the one in the >> > controller >> > > > context. >> > > > >> > > > I will also update the KIP to clarify how it works for >> > BrokerChangeEvent >> > > > processing in more detail. >> > > > >> > > > Thanks, >> > > > Patrick >> > > > >> > > > >> > > > >> > > > On Thu, Oct 11, 2018 at 12:12 PM Jun Rao <j...@confluent.io> wrote: >> > > > >> > > > > Hi, Patrick, >> > > > > >> > > > > Thanks for the KIP. Looks good to me overall and very useful. A >> few >> > > > > comments below. >> > > > > >> > > > > 1. "will reject the requests with smaller broker generation than >> its >> > > > > current generation." Is czxid monotonically increasing? >> > > > > >> > > > > 2. To clarify on the issue of the controller missing a ZK >> watcher. ZK >> > > > > watchers are one-time watchers. Once a watcher is fired, one >> needs to >> > > > > register it again before the watcher can be triggered. So, when >> the >> > > > > controller is busy and a broker goes down and comes up, the first >> > event >> > > > > will trigger the ZK watcher. Since the controller is busy and >> hasn't >> > > > > registered the watcher again, the second event actually won't >> fire. >> > By >> > > > the >> > > > > time the controller reads from ZK, it sees that the broker is >> still >> > > > > registered and thus thinks that nothing has happened to the >> broker, >> > > which >> > > > > is causing the problem. >> > > > > >> > > > > 3. "Handle broker state change: invoke onBrokerFailure(...) first, >> > then >> > > > > invoke onBrokerStartUp(...)". We probably want to be a bit careful >> > > here. >> > > > > Could you clarify the broker list and the broker epoch used when >> > making >> > > > > these calls? We want to prevent the restarted broker from >> receiving a >> > > > > partial replica list on the first LeaderAndIsr request because of >> > this. >> > > > > >> > > > > Thanks, >> > > > > >> > > > > Jun >> > > > > >> > > > > On Wed, Oct 10, 2018 at 12:51 PM, Patrick Huang < >> hzx...@hotmail.com> >> > > > > wrote: >> > > > > >> > > > > > Hey Stanislav, >> > > > > > >> > > > > > Sure. Thanks for your interest in this KIP. I am glad to provide >> > more >> > > > > > detail. >> > > > > > >> > > > > > broker A is initiating a controlled shutdown (restart). The >> > > Controller >> > > > > > sends a StopReplicaRequest but it reaches broker A after it has >> > > started >> > > > > up >> > > > > > again. He therefore stops replicating those partitions even >> though >> > he >> > > > > > should just be starting to >> > > > > > This is right. >> > > > > > >> > > > > > Controller sends a LeaderAndIsrRequest before broker A >> initiates a >> > > > > restart. >> > > > > > Broker A restarts and receives the LeaderAndIsrRequest then. It >> > > > therefore >> > > > > > starts leading for the partitions sent by that request and might >> > stop >> > > > > > leading partitions that it was leading previously. >> > > > > > This was well explained in the linked JIRA, but I cannot >> understand >> > > why >> > > > > > that would happen due to my limited experience. If Broker A >> leads >> > p1 >> > > > and >> > > > > > p2, when would a Controller send a LeaderAndIsrRequest with p1 >> only >> > > and >> > > > > not >> > > > > > want Broker A to drop leadership for p2? >> > > > > > The root cause of the issue is that after a broker just >> restarts, >> > it >> > > > > > relies on the first LeaderAndIsrRequest to populate the >> partition >> > > state >> > > > > and >> > > > > > initializes the highwater mark checkpoint thread. The highwater >> > mark >> > > > > > checkpoint thread will overwrite the highwater mark checkpoint >> file >> > > > based >> > > > > > on the broker's in-memory partition states. In other words, If a >> > > > > partition >> > > > > > that is physically hosted by the broker is missing in the >> in-memory >> > > > > > partition states map, its highwater mark will be lost after the >> > > > highwater >> > > > > > mark checkpoint thread overwrites the file. (Related codes: >> > > > > > >> > https://github.com/apache/kafka/blob/ed3bd79633ae227ad995dafc3d9f38 >> > > > > > 4a5534d4e9/core/src/main/scala/kafka/server/ >> > > > ReplicaManager.scala#L1091) >> > > > > > [https://avatars3.githubusercontent.com/u/47359?s=400&v=4]< >> > > > > > >> > https://github.com/apache/kafka/blob/ed3bd79633ae227ad995dafc3d9f38 >> > > > > > 4a5534d4e9/core/src/main/scala/kafka/server/ >> > > > ReplicaManager.scala#L1091> >> > > > > > >> > > > > > apache/kafka<https://github.com/apache/kafka/blob/ >> > > > > > >> > > > > ed3bd79633ae227ad995dafc3d9f384a5534d4e9/core/src/main/ >> > > > scala/kafka/server/ >> > > > > > ReplicaManager.scala#L1091> >> > > > > > Mirror of Apache Kafka. Contribute to apache/kafka development >> by >> > > > > creating >> > > > > > an account on GitHub. >> > > > > > github.com >> > > > > > >> > > > > > >> > > > > > In your example, assume the first LeaderAndIsrRequest broker A >> > > receives >> > > > > is >> > > > > > the one initiated in the controlled shutdown logic in >> Controller to >> > > > move >> > > > > > leadership away from broker A. This LeaderAndIsrRequest only >> > contains >> > > > > > partitions that broker A leads, not all the partitions that >> broker >> > A >> > > > > hosts >> > > > > > (i.e. no follower partitions), so the highwater mark for the >> > follower >> > > > > > partitions will be lost. Also, the first LeaderAndIsrRequst >> broker >> > A >> > > > > > receives may not necessarily be the one initiated in controlled >> > > > shutdown >> > > > > > logic (e.g. there can be an ongoing preferred leader election), >> > > > although >> > > > > I >> > > > > > think this may not be very common. >> > > > > > >> > > > > > Here the controller will start processing the BrokerChange event >> > > (that >> > > > > says >> > > > > > that broker A shutdown) after the broker has come back up and >> > > > > re-registered >> > > > > > himself in ZK? >> > > > > > How will the Controller miss the restart, won't he subsequently >> > > receive >> > > > > > another ZK event saying that broker A has come back up? >> > > > > > Controller will not miss the BrokerChange event and actually >> there >> > > will >> > > > > be >> > > > > > two BrokerChange events fired in this case (one for broker >> > > > deregistration >> > > > > > in zk and one for registration). However, when processing the >> > > > > > BrokerChangeEvent, controller needs to do a read from zookeeper >> to >> > > get >> > > > > back >> > > > > > the current brokers in the cluster and if the bounced broker >> > already >> > > > > joined >> > > > > > the cluster by this time, controller will not know this broker >> has >> > > been >> > > > > > bounced because it sees no diff between zk and its in-memory >> cache. >> > > So >> > > > > > basically both of the BrokerChange event processing become >> no-op. >> > > > > > >> > > > > > >> > > > > > Hope that I answer your questions. Feel free to follow up if I >> am >> > > > missing >> > > > > > something. >> > > > > > >> > > > > > >> > > > > > Thanks, >> > > > > > Zhanxiang (Patrick) Huang >> > > > > > >> > > > > > ________________________________ >> > > > > > From: Stanislav Kozlovski <stanis...@confluent.io> >> > > > > > Sent: Wednesday, October 10, 2018 7:22 >> > > > > > To: dev@kafka.apache.org >> > > > > > Subject: Re: [DISCUSS] KIP-380: Detect outdated control requests >> > and >> > > > > > bounced brokers using broker generation >> > > > > > >> > > > > > Hi Patrick, >> > > > > > >> > > > > > Thanks for the KIP! Fixing such correctness issues is always >> very >> > > > > welcome - >> > > > > > they're commonly hard to diagnose and debug when they happen in >> > > > > production. >> > > > > > >> > > > > > I was wondering if I understood the potential correctness issues >> > > > > correctly. >> > > > > > Here is what I got: >> > > > > > >> > > > > > >> > > > > > - If a broker bounces during controlled shutdown, the bounced >> > > broker >> > > > > may >> > > > > > accidentally process its earlier generation’s >> StopReplicaRequest >> > > > sent >> > > > > > from >> > > > > > the active controller for one of its follower replicas, >> leaving >> > > the >> > > > > > replica >> > > > > > offline while its remaining replicas may stay online >> > > > > > >> > > > > > broker A is initiating a controlled shutdown (restart). The >> > > Controller >> > > > > > sends a StopReplicaRequest but it reaches broker A after it has >> > > started >> > > > > up >> > > > > > again. He therefore stops replicating those partitions even >> though >> > he >> > > > > > should just be starting to >> > > > > > >> > > > > > >> > > > > > - If the first LeaderAndIsrRequest that a broker processes is >> > sent >> > > > by >> > > > > > the active controller before its startup, the broker will >> > > overwrite >> > > > > the >> > > > > > high watermark checkpoint file and may cause incorrect >> > truncation >> > > ( >> > > > > > KAFKA-7235 <https://issues.apache.org/jira/browse/KAFKA-7235 >> >) >> > > > > > >> > > > > > Controller sends a LeaderAndIsrRequest before broker A >> initiates a >> > > > > restart. >> > > > > > Broker A restarts and receives the LeaderAndIsrRequest then. It >> > > > therefore >> > > > > > starts leading for the partitions sent by that request and might >> > stop >> > > > > > leading partitions that it was leading previously. >> > > > > > This was well explained in the linked JIRA, but I cannot >> understand >> > > why >> > > > > > that would happen due to my limited experience. If Broker A >> leads >> > p1 >> > > > and >> > > > > > p2, when would a Controller send a LeaderAndIsrRequest with p1 >> only >> > > and >> > > > > not >> > > > > > want Broker A to drop leadership for p2? >> > > > > > >> > > > > > >> > > > > > - If a broker bounces very quickly, the controller may start >> > > > > processing >> > > > > > the BrokerChange event after the broker already re-registers >> > > itself >> > > > in >> > > > > > zk. >> > > > > > In this case, controller will miss the broker restart and >> will >> > not >> > > > > send >> > > > > > any >> > > > > > requests to the broker for initialization. The broker will >> not >> > be >> > > > able >> > > > > > to >> > > > > > accept traffics. >> > > > > > >> > > > > > Here the controller will start processing the BrokerChange event >> > > (that >> > > > > says >> > > > > > that broker A shutdown) after the broker has come back up and >> > > > > re-registered >> > > > > > himself in ZK? >> > > > > > How will the Controller miss the restart, won't he subsequently >> > > receive >> > > > > > another ZK event saying that broker A has come back up? >> > > > > > >> > > > > > >> > > > > > Could we explain these potential problems in a bit more detail >> just >> > > so >> > > > > they >> > > > > > could be more easily digestable by novices? >> > > > > > >> > > > > > Thanks, >> > > > > > Stanislav >> > > > > > >> > > > > > On Wed, Oct 10, 2018 at 9:21 AM Dong Lin <lindon...@gmail.com> >> > > wrote: >> > > > > > >> > > > > > > Hey Patrick, >> > > > > > > >> > > > > > > Thanks much for the KIP. The KIP is very well written. >> > > > > > > >> > > > > > > LGTM. +1 (binding) >> > > > > > > >> > > > > > > Thanks, >> > > > > > > Dong >> > > > > > > >> > > > > > > >> > > > > > > On Tue, Oct 9, 2018 at 11:46 PM Patrick Huang < >> > hzx...@hotmail.com> >> > > > > > wrote: >> > > > > > > >> > > > > > > > Hi All, >> > > > > > > > >> > > > > > > > Please find the below KIP which proposes the concept of >> broker >> > > > > > generation >> > > > > > > > to resolve issues caused by controller missing broker state >> > > changes >> > > > > and >> > > > > > > > broker processing outdated control requests. >> > > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP- >> > > > > > 380%3A+Detect+outdated+control+requests+and+bounced+ >> > > > brokers+using+broker+ >> > > > > > generation >> > > > > > > > >> > > > > > > > All comments are appreciated. >> > > > > > > > >> > > > > > > > Best, >> > > > > > > > Zhanxiang (Patrick) Huang >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > > >> > > > > > -- >> > > > > > Best, >> > > > > > Stanislav >> > > > > > >> > > > > >> > > > >> > > >> > >> >
