KAFKA-2462: allow modifying soft limit for open files in Kafka startup script#164
Closed
gwenshap wants to merge 1 commit into
Closed
KAFKA-2462: allow modifying soft limit for open files in Kafka startup script#164gwenshap wants to merge 1 commit into
gwenshap wants to merge 1 commit into
Conversation
|
kafka-trunk-git-pr #207 SUCCESS |
Contributor
There was a problem hiding this comment.
I think this doesn't work if you provide them in the reverse order (i.e., --soft-file-limit <number> -daemon, which I would assume would be possible)? I think you'd need a while loop around this.
Contributor
|
@gwenshap Would you like to continue working on this PR? |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
Contributor
|
Closing for cleanup. |
efeg
added a commit
to efeg/kafka
that referenced
this pull request
Jan 29, 2020
efeg
pushed a commit
to efeg/kafka
that referenced
this pull request
May 26, 2021
…king the controlled shutdown (apache#164) TICKET = N/A LI_DESCRIPTION = The partition info is quite useful and we want to show it by default. Thus I'm upgrading the log level to INFO. EXIT_CRITERIA = N/A
wyuka
pushed a commit
to wyuka/kafka
that referenced
this pull request
Jan 19, 2022
…king the controlled shutdown (apache#164) TICKET = N/A LI_DESCRIPTION = The partition info is quite useful and we want to show it by default. Thus I'm upgrading the log level to INFO. EXIT_CRITERIA = N/A
wyuka
added a commit
to wyuka/kafka
that referenced
this pull request
Jan 25, 2022
* [LI-HOTFIX] Add safety check to controlled shutdown (apache#103) The controller will not allow a broker to initiate controlled shutdown unless the shutdown is unlikely to lead to a topic-partition ISR shrinking below the min.insync.replicas setting for that topic. This new functionality is optional and disabled by default. The following new configs have been added: * controlled.shutdown.safety.check.enable: Enable this safety check for any ControlledShutdownRequest handled by the controller. * controlled.shutdown.safety.check.redundancy.factor: Additional replicas above min.insync.replicas that are required for any topic-partition to allow shutdown of a broker. Default is 2, so if a topic has min.insync.replicas=2, then the ISR must have 4 replicas to allow shutdown. By default a broker attempting shutdown will only send ControlledShutdownRequest to the controller 3 times with a 5 second sleep in between. If it is desired to never skip controlled shutdown, controlled.shutdown.max.retries can be increased to a much larger value than 3. It can be set to 2147483647 (Integer.MAX_VALUE) to effectively retry forever. A controller integration test has been added which exercises this code with a simulated three node cluster with a topic configured with min.insync.replicas=2, allowing only one broker to shut down at a time. In ControllerContext, instead of maintaining a list of broker ids that have begun shutting down called shuttingDownBrokers, a map of brokerId -> brokerEpoch. This will make it easier to keep this map and broker presence notifications (via zookeeper) in sync. When a ControlledShutdown request is allowed to begin, it is recorded in zookeeper under /brokers/shutdown/<broker-id>, with the value of the broker epoch. On controller initialization this is loaded into controllerContext.shuttingDownBrokers. When a broker has restarted, it will come back with a new broker epoch which will take precedence over the epoch in controllerContext.shuttingDownBrokers. The controller now needs to know the min.insync.replicas of every topic. This is done by a combination of DynamicConfigManager, a full scan of all topic configs on controller failover, and a /topics znode watcher. All update a map in ControllerContext. In order to allow skipping the safety check, this patch adds LiControlledShutdownSkipSafetyCheckRequest. It takes a brokerId and brokerEpoch. When sent to the controller, the controller will record it in a map in memory. After handling that request, the controller will ignore the results of the safety check for any request with that brokerId and epoch <= the epoch provided via LiControlledShutdownSkipSafetyCheckRequest. This is not persisted to zookeeper since this is an emergency action. If the controller moves then an admin can just send the request again. This request is authorized as a cluster action. Since LiControlledShutdownSkipSafetyCheck is not submitted to upstream, it has been added with an ApiKey of 1000. The largest ApiKey in upstream is 47. This new API is only meant to be used by AdminClient and is not meant to be an inter-broker request. So this sets the precedent that if we have an admin-only API we want to add that isn't upstream yet, we can give it an ApiKey of 1000+. Then if this internal-only admin API does make it upstream, migrating our tools to it will be easy since usage should be minimal. This required fixing a couple of tests that made assumptions about ApiKeys, namely that the array of api keys now has gaps in it. Now ApiKeys#hasKeys returns true if the key is within range *and* the entry in the ID_TO_TYPE array is not null. * Fix build. * Fix integ test. * [LI-HOTFIX] Skip shutdown safety check at epoch already checked (apache#157) TICKET = N/A LI_DESCRIPTION = This makes a couple of changes to address some deficiencies discovered with this safety check when using it in a large, busy cluster. * Skip the check if already approved for the requested epoch * Define redundancy factor property as the required result of a broker shutdown, not the required state before shutdown, allowing the shutdown of lagging replicas. * Change default redundancy factor from 2 => 1 In c337e47 (PR apache#103) a shutdown safety check was introduced. In testing this in a large cluster, we found a problem. If the controller sees that brokerId 1 is safe to shut down at brokerEpoch 99, it will add that broker to `controllerContext.shuttingDownBrokerIds`. But it might still not allow shutdown because some leadership transitions must happen first. In the process of performing those leadership transitions, brokerId 1 may receive StopReplica, which would cause it to leave the ISR. Subsequent ControlledShutdownRequest attempts would then fail the `safeToShutdown` safety check, so the broker would never be allowed to shut down. This patch adjusts the implementation of `safeToShutdown` slightly. Now, if the controller has said that a broker is safe to shut down at a given epoch, it will always allow that shutdown for that epoch without performing any additional checks. This allows the full controlled shutdown process to complete which in a large, busy cluster may take several rounds of `ControlledShutdownRequest`. In addition, Lucas pointed out that if a broker which is attempting to shutdown is already out of ISR, then it is probably pretty harmless to allow it to shut down if the safety check would have allowed it to when there are no under-replicated partitions. So we changed the logic a little further. ControlledShutdownSafetyCheckEnableProp is now defined as, "Shutdown will be allowed if removing the candidate broker will leave min.insync.replicas + ControlledShutdownSafetyCheckEnableProp replicas in the ISR." The default redundancy factor is changed to 1. EXIT_CRITERIA = N/A * [LI-HOTFIX] Upgrade log level to INFO to show which partition is blocking the controlled shutdown (apache#164) TICKET = N/A LI_DESCRIPTION = The partition info is quite useful and we want to show it by default. Thus I'm upgrading the log level to INFO. EXIT_CRITERIA = N/A * [LI-HOTFIX] Topics being deleted shouldn't block the controlled shutdown (apache#181) TICKET = LIKAFKA-36940 LI_DESCRIPTION = In clusters with ongoing topic deletions, we found that the controlled shutdown can be blocked by topics that are being deleted. This PR skips the check for topics being deleted so that they don't block the shutdown. EXIT_CRITERIA = Never * [LI-HOTFIX] Ensure that the controller has fresh values for min.insync.replicas after topic-level configs are dropped. (apache#207) TICKET = CM-79574 LI_DESCRIPTION = The following series of events causes the Controller to keep stale config value for min.insync.replicas config. This can cause a broker shutdown to be blocked when ISR-based shutdown logic is adopted. 1. Given a topic T, set its topic-level min.insync.replicas config value to XXX. 2. Then remove the topic-level min.insync.replicas config value of topic T Currently, step-1 will cause the controller update its "controllerContext.topicMinIsrConfig" value for the topic T to XXX. However, step-2 will not remove the config value XXX from "controllerContext.topicMinIsrConfig". As a result, Controller will cache a stale config value of XXX for topic T. The expected behavior was for the topic to use the default config value after its topic-level config was removed. * The above events can cause the ISR-based shutdown logic to block shutdown. For example, 1. Set the min.insync.replicas=2 for topic T. 2. Remvoe topic-level min.insync.replicas config value for topic T. 3. Use redundancy factor of 1 for ISR-based shutdown logic. 4. Observe that controller will block shutdown for a partition of the topic with the following log: ``` T-2 has min.insync.replicas=2 and a redundancy factor of 1. Removing broker XXX will leave 2 live replicas in the ISR. ``` EXIT_CRITERIA = When this change is merged in upstream and the corresponding change is pulled into a future release. * Disable test where shutdown is Kafka shutdown is attempted when ZK is unavailable. * Remove incorrect filtering logic in ControllerContext::removeLiveBrokers Co-authored-by: Kyle Ambroff-Kao <kyle@ambroffkao.com> Co-authored-by: Lucas Wang <luwang@linkedin.com> Co-authored-by: Adem Efe Gencer <agencer@linkedin.com>
wyuka
added a commit
to wyuka/kafka
that referenced
this pull request
Mar 4, 2022
* [LI-HOTFIX] Add safety check to controlled shutdown (apache#103) The controller will not allow a broker to initiate controlled shutdown unless the shutdown is unlikely to lead to a topic-partition ISR shrinking below the min.insync.replicas setting for that topic. This new functionality is optional and disabled by default. The following new configs have been added: * controlled.shutdown.safety.check.enable: Enable this safety check for any ControlledShutdownRequest handled by the controller. * controlled.shutdown.safety.check.redundancy.factor: Additional replicas above min.insync.replicas that are required for any topic-partition to allow shutdown of a broker. Default is 2, so if a topic has min.insync.replicas=2, then the ISR must have 4 replicas to allow shutdown. By default a broker attempting shutdown will only send ControlledShutdownRequest to the controller 3 times with a 5 second sleep in between. If it is desired to never skip controlled shutdown, controlled.shutdown.max.retries can be increased to a much larger value than 3. It can be set to 2147483647 (Integer.MAX_VALUE) to effectively retry forever. A controller integration test has been added which exercises this code with a simulated three node cluster with a topic configured with min.insync.replicas=2, allowing only one broker to shut down at a time. In ControllerContext, instead of maintaining a list of broker ids that have begun shutting down called shuttingDownBrokers, a map of brokerId -> brokerEpoch. This will make it easier to keep this map and broker presence notifications (via zookeeper) in sync. When a ControlledShutdown request is allowed to begin, it is recorded in zookeeper under /brokers/shutdown/<broker-id>, with the value of the broker epoch. On controller initialization this is loaded into controllerContext.shuttingDownBrokers. When a broker has restarted, it will come back with a new broker epoch which will take precedence over the epoch in controllerContext.shuttingDownBrokers. The controller now needs to know the min.insync.replicas of every topic. This is done by a combination of DynamicConfigManager, a full scan of all topic configs on controller failover, and a /topics znode watcher. All update a map in ControllerContext. In order to allow skipping the safety check, this patch adds LiControlledShutdownSkipSafetyCheckRequest. It takes a brokerId and brokerEpoch. When sent to the controller, the controller will record it in a map in memory. After handling that request, the controller will ignore the results of the safety check for any request with that brokerId and epoch <= the epoch provided via LiControlledShutdownSkipSafetyCheckRequest. This is not persisted to zookeeper since this is an emergency action. If the controller moves then an admin can just send the request again. This request is authorized as a cluster action. Since LiControlledShutdownSkipSafetyCheck is not submitted to upstream, it has been added with an ApiKey of 1000. The largest ApiKey in upstream is 47. This new API is only meant to be used by AdminClient and is not meant to be an inter-broker request. So this sets the precedent that if we have an admin-only API we want to add that isn't upstream yet, we can give it an ApiKey of 1000+. Then if this internal-only admin API does make it upstream, migrating our tools to it will be easy since usage should be minimal. This required fixing a couple of tests that made assumptions about ApiKeys, namely that the array of api keys now has gaps in it. Now ApiKeys#hasKeys returns true if the key is within range *and* the entry in the ID_TO_TYPE array is not null. * Fix build. * Fix integ test. * [LI-HOTFIX] Skip shutdown safety check at epoch already checked (apache#157) TICKET = N/A LI_DESCRIPTION = This makes a couple of changes to address some deficiencies discovered with this safety check when using it in a large, busy cluster. * Skip the check if already approved for the requested epoch * Define redundancy factor property as the required result of a broker shutdown, not the required state before shutdown, allowing the shutdown of lagging replicas. * Change default redundancy factor from 2 => 1 In c337e47 (PR apache#103) a shutdown safety check was introduced. In testing this in a large cluster, we found a problem. If the controller sees that brokerId 1 is safe to shut down at brokerEpoch 99, it will add that broker to `controllerContext.shuttingDownBrokerIds`. But it might still not allow shutdown because some leadership transitions must happen first. In the process of performing those leadership transitions, brokerId 1 may receive StopReplica, which would cause it to leave the ISR. Subsequent ControlledShutdownRequest attempts would then fail the `safeToShutdown` safety check, so the broker would never be allowed to shut down. This patch adjusts the implementation of `safeToShutdown` slightly. Now, if the controller has said that a broker is safe to shut down at a given epoch, it will always allow that shutdown for that epoch without performing any additional checks. This allows the full controlled shutdown process to complete which in a large, busy cluster may take several rounds of `ControlledShutdownRequest`. In addition, Lucas pointed out that if a broker which is attempting to shutdown is already out of ISR, then it is probably pretty harmless to allow it to shut down if the safety check would have allowed it to when there are no under-replicated partitions. So we changed the logic a little further. ControlledShutdownSafetyCheckEnableProp is now defined as, "Shutdown will be allowed if removing the candidate broker will leave min.insync.replicas + ControlledShutdownSafetyCheckEnableProp replicas in the ISR." The default redundancy factor is changed to 1. EXIT_CRITERIA = N/A * [LI-HOTFIX] Upgrade log level to INFO to show which partition is blocking the controlled shutdown (apache#164) TICKET = N/A LI_DESCRIPTION = The partition info is quite useful and we want to show it by default. Thus I'm upgrading the log level to INFO. EXIT_CRITERIA = N/A * [LI-HOTFIX] Topics being deleted shouldn't block the controlled shutdown (apache#181) TICKET = LIKAFKA-36940 LI_DESCRIPTION = In clusters with ongoing topic deletions, we found that the controlled shutdown can be blocked by topics that are being deleted. This PR skips the check for topics being deleted so that they don't block the shutdown. EXIT_CRITERIA = Never * [LI-HOTFIX] Ensure that the controller has fresh values for min.insync.replicas after topic-level configs are dropped. (apache#207) TICKET = CM-79574 LI_DESCRIPTION = The following series of events causes the Controller to keep stale config value for min.insync.replicas config. This can cause a broker shutdown to be blocked when ISR-based shutdown logic is adopted. 1. Given a topic T, set its topic-level min.insync.replicas config value to XXX. 2. Then remove the topic-level min.insync.replicas config value of topic T Currently, step-1 will cause the controller update its "controllerContext.topicMinIsrConfig" value for the topic T to XXX. However, step-2 will not remove the config value XXX from "controllerContext.topicMinIsrConfig". As a result, Controller will cache a stale config value of XXX for topic T. The expected behavior was for the topic to use the default config value after its topic-level config was removed. * The above events can cause the ISR-based shutdown logic to block shutdown. For example, 1. Set the min.insync.replicas=2 for topic T. 2. Remvoe topic-level min.insync.replicas config value for topic T. 3. Use redundancy factor of 1 for ISR-based shutdown logic. 4. Observe that controller will block shutdown for a partition of the topic with the following log: ``` T-2 has min.insync.replicas=2 and a redundancy factor of 1. Removing broker XXX will leave 2 live replicas in the ISR. ``` EXIT_CRITERIA = When this change is merged in upstream and the corresponding change is pulled into a future release. * Disable test where shutdown is Kafka shutdown is attempted when ZK is unavailable. * Remove incorrect filtering logic in ControllerContext::removeLiveBrokers Co-authored-by: Kyle Ambroff-Kao <kyle@ambroffkao.com> Co-authored-by: Lucas Wang <luwang@linkedin.com> Co-authored-by: Adem Efe Gencer <agencer@linkedin.com>
wyuka
added a commit
to wyuka/kafka
that referenced
this pull request
Mar 28, 2022
* [LI-HOTFIX] Add safety check to controlled shutdown (apache#103) The controller will not allow a broker to initiate controlled shutdown unless the shutdown is unlikely to lead to a topic-partition ISR shrinking below the min.insync.replicas setting for that topic. This new functionality is optional and disabled by default. The following new configs have been added: * controlled.shutdown.safety.check.enable: Enable this safety check for any ControlledShutdownRequest handled by the controller. * controlled.shutdown.safety.check.redundancy.factor: Additional replicas above min.insync.replicas that are required for any topic-partition to allow shutdown of a broker. Default is 2, so if a topic has min.insync.replicas=2, then the ISR must have 4 replicas to allow shutdown. By default a broker attempting shutdown will only send ControlledShutdownRequest to the controller 3 times with a 5 second sleep in between. If it is desired to never skip controlled shutdown, controlled.shutdown.max.retries can be increased to a much larger value than 3. It can be set to 2147483647 (Integer.MAX_VALUE) to effectively retry forever. A controller integration test has been added which exercises this code with a simulated three node cluster with a topic configured with min.insync.replicas=2, allowing only one broker to shut down at a time. In ControllerContext, instead of maintaining a list of broker ids that have begun shutting down called shuttingDownBrokers, a map of brokerId -> brokerEpoch. This will make it easier to keep this map and broker presence notifications (via zookeeper) in sync. When a ControlledShutdown request is allowed to begin, it is recorded in zookeeper under /brokers/shutdown/<broker-id>, with the value of the broker epoch. On controller initialization this is loaded into controllerContext.shuttingDownBrokers. When a broker has restarted, it will come back with a new broker epoch which will take precedence over the epoch in controllerContext.shuttingDownBrokers. The controller now needs to know the min.insync.replicas of every topic. This is done by a combination of DynamicConfigManager, a full scan of all topic configs on controller failover, and a /topics znode watcher. All update a map in ControllerContext. In order to allow skipping the safety check, this patch adds LiControlledShutdownSkipSafetyCheckRequest. It takes a brokerId and brokerEpoch. When sent to the controller, the controller will record it in a map in memory. After handling that request, the controller will ignore the results of the safety check for any request with that brokerId and epoch <= the epoch provided via LiControlledShutdownSkipSafetyCheckRequest. This is not persisted to zookeeper since this is an emergency action. If the controller moves then an admin can just send the request again. This request is authorized as a cluster action. Since LiControlledShutdownSkipSafetyCheck is not submitted to upstream, it has been added with an ApiKey of 1000. The largest ApiKey in upstream is 47. This new API is only meant to be used by AdminClient and is not meant to be an inter-broker request. So this sets the precedent that if we have an admin-only API we want to add that isn't upstream yet, we can give it an ApiKey of 1000+. Then if this internal-only admin API does make it upstream, migrating our tools to it will be easy since usage should be minimal. This required fixing a couple of tests that made assumptions about ApiKeys, namely that the array of api keys now has gaps in it. Now ApiKeys#hasKeys returns true if the key is within range *and* the entry in the ID_TO_TYPE array is not null. * Fix build. * Fix integ test. * [LI-HOTFIX] Skip shutdown safety check at epoch already checked (apache#157) TICKET = N/A LI_DESCRIPTION = This makes a couple of changes to address some deficiencies discovered with this safety check when using it in a large, busy cluster. * Skip the check if already approved for the requested epoch * Define redundancy factor property as the required result of a broker shutdown, not the required state before shutdown, allowing the shutdown of lagging replicas. * Change default redundancy factor from 2 => 1 In c337e47 (PR apache#103) a shutdown safety check was introduced. In testing this in a large cluster, we found a problem. If the controller sees that brokerId 1 is safe to shut down at brokerEpoch 99, it will add that broker to `controllerContext.shuttingDownBrokerIds`. But it might still not allow shutdown because some leadership transitions must happen first. In the process of performing those leadership transitions, brokerId 1 may receive StopReplica, which would cause it to leave the ISR. Subsequent ControlledShutdownRequest attempts would then fail the `safeToShutdown` safety check, so the broker would never be allowed to shut down. This patch adjusts the implementation of `safeToShutdown` slightly. Now, if the controller has said that a broker is safe to shut down at a given epoch, it will always allow that shutdown for that epoch without performing any additional checks. This allows the full controlled shutdown process to complete which in a large, busy cluster may take several rounds of `ControlledShutdownRequest`. In addition, Lucas pointed out that if a broker which is attempting to shutdown is already out of ISR, then it is probably pretty harmless to allow it to shut down if the safety check would have allowed it to when there are no under-replicated partitions. So we changed the logic a little further. ControlledShutdownSafetyCheckEnableProp is now defined as, "Shutdown will be allowed if removing the candidate broker will leave min.insync.replicas + ControlledShutdownSafetyCheckEnableProp replicas in the ISR." The default redundancy factor is changed to 1. EXIT_CRITERIA = N/A * [LI-HOTFIX] Upgrade log level to INFO to show which partition is blocking the controlled shutdown (apache#164) TICKET = N/A LI_DESCRIPTION = The partition info is quite useful and we want to show it by default. Thus I'm upgrading the log level to INFO. EXIT_CRITERIA = N/A * [LI-HOTFIX] Topics being deleted shouldn't block the controlled shutdown (apache#181) TICKET = LIKAFKA-36940 LI_DESCRIPTION = In clusters with ongoing topic deletions, we found that the controlled shutdown can be blocked by topics that are being deleted. This PR skips the check for topics being deleted so that they don't block the shutdown. EXIT_CRITERIA = Never * [LI-HOTFIX] Ensure that the controller has fresh values for min.insync.replicas after topic-level configs are dropped. (apache#207) TICKET = CM-79574 LI_DESCRIPTION = The following series of events causes the Controller to keep stale config value for min.insync.replicas config. This can cause a broker shutdown to be blocked when ISR-based shutdown logic is adopted. 1. Given a topic T, set its topic-level min.insync.replicas config value to XXX. 2. Then remove the topic-level min.insync.replicas config value of topic T Currently, step-1 will cause the controller update its "controllerContext.topicMinIsrConfig" value for the topic T to XXX. However, step-2 will not remove the config value XXX from "controllerContext.topicMinIsrConfig". As a result, Controller will cache a stale config value of XXX for topic T. The expected behavior was for the topic to use the default config value after its topic-level config was removed. * The above events can cause the ISR-based shutdown logic to block shutdown. For example, 1. Set the min.insync.replicas=2 for topic T. 2. Remvoe topic-level min.insync.replicas config value for topic T. 3. Use redundancy factor of 1 for ISR-based shutdown logic. 4. Observe that controller will block shutdown for a partition of the topic with the following log: ``` T-2 has min.insync.replicas=2 and a redundancy factor of 1. Removing broker XXX will leave 2 live replicas in the ISR. ``` EXIT_CRITERIA = When this change is merged in upstream and the corresponding change is pulled into a future release. * Disable test where shutdown is Kafka shutdown is attempted when ZK is unavailable. * Remove incorrect filtering logic in ControllerContext::removeLiveBrokers Co-authored-by: Kyle Ambroff-Kao <kyle@ambroffkao.com> Co-authored-by: Lucas Wang <luwang@linkedin.com> Co-authored-by: Adem Efe Gencer <agencer@linkedin.com> * [LI-HOTFIX] Improve the testControlledShutdownRejectRequestForAvailabilityRisk test (apache#141) TICKET = N/A LI_DESCRIPTION = In the test testControlledShutdownRejectRequestForAvailabilityRisk, sometimes the epoch for the shutting down broker becomes 50. This PR improves the assertions so that the test doesn't fail when such a condition happens. EXIT_CRITERIA = N/A
wyuka
added a commit
to wyuka/kafka
that referenced
this pull request
Jun 16, 2022
* [LI-HOTFIX] Add safety check to controlled shutdown (apache#103) The controller will not allow a broker to initiate controlled shutdown unless the shutdown is unlikely to lead to a topic-partition ISR shrinking below the min.insync.replicas setting for that topic. This new functionality is optional and disabled by default. The following new configs have been added: * controlled.shutdown.safety.check.enable: Enable this safety check for any ControlledShutdownRequest handled by the controller. * controlled.shutdown.safety.check.redundancy.factor: Additional replicas above min.insync.replicas that are required for any topic-partition to allow shutdown of a broker. Default is 2, so if a topic has min.insync.replicas=2, then the ISR must have 4 replicas to allow shutdown. By default a broker attempting shutdown will only send ControlledShutdownRequest to the controller 3 times with a 5 second sleep in between. If it is desired to never skip controlled shutdown, controlled.shutdown.max.retries can be increased to a much larger value than 3. It can be set to 2147483647 (Integer.MAX_VALUE) to effectively retry forever. A controller integration test has been added which exercises this code with a simulated three node cluster with a topic configured with min.insync.replicas=2, allowing only one broker to shut down at a time. In ControllerContext, instead of maintaining a list of broker ids that have begun shutting down called shuttingDownBrokers, a map of brokerId -> brokerEpoch. This will make it easier to keep this map and broker presence notifications (via zookeeper) in sync. When a ControlledShutdown request is allowed to begin, it is recorded in zookeeper under /brokers/shutdown/<broker-id>, with the value of the broker epoch. On controller initialization this is loaded into controllerContext.shuttingDownBrokers. When a broker has restarted, it will come back with a new broker epoch which will take precedence over the epoch in controllerContext.shuttingDownBrokers. The controller now needs to know the min.insync.replicas of every topic. This is done by a combination of DynamicConfigManager, a full scan of all topic configs on controller failover, and a /topics znode watcher. All update a map in ControllerContext. In order to allow skipping the safety check, this patch adds LiControlledShutdownSkipSafetyCheckRequest. It takes a brokerId and brokerEpoch. When sent to the controller, the controller will record it in a map in memory. After handling that request, the controller will ignore the results of the safety check for any request with that brokerId and epoch <= the epoch provided via LiControlledShutdownSkipSafetyCheckRequest. This is not persisted to zookeeper since this is an emergency action. If the controller moves then an admin can just send the request again. This request is authorized as a cluster action. Since LiControlledShutdownSkipSafetyCheck is not submitted to upstream, it has been added with an ApiKey of 1000. The largest ApiKey in upstream is 47. This new API is only meant to be used by AdminClient and is not meant to be an inter-broker request. So this sets the precedent that if we have an admin-only API we want to add that isn't upstream yet, we can give it an ApiKey of 1000+. Then if this internal-only admin API does make it upstream, migrating our tools to it will be easy since usage should be minimal. This required fixing a couple of tests that made assumptions about ApiKeys, namely that the array of api keys now has gaps in it. Now ApiKeys#hasKeys returns true if the key is within range *and* the entry in the ID_TO_TYPE array is not null. * Fix build. * Fix integ test. * [LI-HOTFIX] Skip shutdown safety check at epoch already checked (apache#157) TICKET = N/A LI_DESCRIPTION = This makes a couple of changes to address some deficiencies discovered with this safety check when using it in a large, busy cluster. * Skip the check if already approved for the requested epoch * Define redundancy factor property as the required result of a broker shutdown, not the required state before shutdown, allowing the shutdown of lagging replicas. * Change default redundancy factor from 2 => 1 In c337e47 (PR apache#103) a shutdown safety check was introduced. In testing this in a large cluster, we found a problem. If the controller sees that brokerId 1 is safe to shut down at brokerEpoch 99, it will add that broker to `controllerContext.shuttingDownBrokerIds`. But it might still not allow shutdown because some leadership transitions must happen first. In the process of performing those leadership transitions, brokerId 1 may receive StopReplica, which would cause it to leave the ISR. Subsequent ControlledShutdownRequest attempts would then fail the `safeToShutdown` safety check, so the broker would never be allowed to shut down. This patch adjusts the implementation of `safeToShutdown` slightly. Now, if the controller has said that a broker is safe to shut down at a given epoch, it will always allow that shutdown for that epoch without performing any additional checks. This allows the full controlled shutdown process to complete which in a large, busy cluster may take several rounds of `ControlledShutdownRequest`. In addition, Lucas pointed out that if a broker which is attempting to shutdown is already out of ISR, then it is probably pretty harmless to allow it to shut down if the safety check would have allowed it to when there are no under-replicated partitions. So we changed the logic a little further. ControlledShutdownSafetyCheckEnableProp is now defined as, "Shutdown will be allowed if removing the candidate broker will leave min.insync.replicas + ControlledShutdownSafetyCheckEnableProp replicas in the ISR." The default redundancy factor is changed to 1. EXIT_CRITERIA = N/A * [LI-HOTFIX] Upgrade log level to INFO to show which partition is blocking the controlled shutdown (apache#164) TICKET = N/A LI_DESCRIPTION = The partition info is quite useful and we want to show it by default. Thus I'm upgrading the log level to INFO. EXIT_CRITERIA = N/A * [LI-HOTFIX] Topics being deleted shouldn't block the controlled shutdown (apache#181) TICKET = LIKAFKA-36940 LI_DESCRIPTION = In clusters with ongoing topic deletions, we found that the controlled shutdown can be blocked by topics that are being deleted. This PR skips the check for topics being deleted so that they don't block the shutdown. EXIT_CRITERIA = Never * [LI-HOTFIX] Ensure that the controller has fresh values for min.insync.replicas after topic-level configs are dropped. (apache#207) TICKET = CM-79574 LI_DESCRIPTION = The following series of events causes the Controller to keep stale config value for min.insync.replicas config. This can cause a broker shutdown to be blocked when ISR-based shutdown logic is adopted. 1. Given a topic T, set its topic-level min.insync.replicas config value to XXX. 2. Then remove the topic-level min.insync.replicas config value of topic T Currently, step-1 will cause the controller update its "controllerContext.topicMinIsrConfig" value for the topic T to XXX. However, step-2 will not remove the config value XXX from "controllerContext.topicMinIsrConfig". As a result, Controller will cache a stale config value of XXX for topic T. The expected behavior was for the topic to use the default config value after its topic-level config was removed. * The above events can cause the ISR-based shutdown logic to block shutdown. For example, 1. Set the min.insync.replicas=2 for topic T. 2. Remvoe topic-level min.insync.replicas config value for topic T. 3. Use redundancy factor of 1 for ISR-based shutdown logic. 4. Observe that controller will block shutdown for a partition of the topic with the following log: ``` T-2 has min.insync.replicas=2 and a redundancy factor of 1. Removing broker XXX will leave 2 live replicas in the ISR. ``` EXIT_CRITERIA = When this change is merged in upstream and the corresponding change is pulled into a future release. * Disable test where shutdown is Kafka shutdown is attempted when ZK is unavailable. * Remove incorrect filtering logic in ControllerContext::removeLiveBrokers Co-authored-by: Kyle Ambroff-Kao <kyle@ambroffkao.com> Co-authored-by: Lucas Wang <luwang@linkedin.com> Co-authored-by: Adem Efe Gencer <agencer@linkedin.com> * [LI-HOTFIX] Improve the testControlledShutdownRejectRequestForAvailabilityRisk test (apache#141) TICKET = N/A LI_DESCRIPTION = In the test testControlledShutdownRejectRequestForAvailabilityRisk, sometimes the epoch for the shutting down broker becomes 50. This PR improves the assertions so that the test doesn't fail when such a condition happens. EXIT_CRITERIA = N/A
davide-armand
pushed a commit
to aiven/kafka
that referenced
this pull request
Dec 1, 2025
Signed-off-by: Greg Harris <greg.harris@aiven.io>
fvaleri
added a commit
to fvaleri/kafka
that referenced
this pull request
May 15, 2026
* Rename Mirror to ClusterMirror in API and classes Signed-off-by: Federico Valeri <fedevaleri@gmail.com> * Fix mirror partition stuck in STOPPING state When the leader epoch is already above the target requested by buildLocalEpochBumpTargets (e.g. epoch 11 vs target 0), the BumpLeaderEpochs request produces no new metadata record. Since maybeCompletePendingEpochBumps() only ran inside onMetadataUpdate(), the completion future was never resolved and the STOPPING chain blocked indefinitely. Call maybeCompletePendingEpochBumps() right after adding to the pending list so the already-met condition is detected immediately. Signed-off-by: Federico Valeri <fedevaleri@gmail.com> * Fix system tests Signed-off-by: Federico Valeri <fedevaleri@gmail.com> * More refactoring Signed-off-by: Federico Valeri <fedevaleri@gmail.com> * Add comment Signed-off-by: Federico Valeri <fedevaleri@gmail.com> --------- Signed-off-by: Federico Valeri <fedevaleri@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.