You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "cmccabe (via GitHub)" <gi...@apache.org> on 2024/12/02 22:12:32 UTC
[PR] KAFKA-18138: The controller must add all extant brokers to BrokerHeartbeatTracker when activating [kafka]
cmccabe opened a new pull request, #18009:
URL: https://github.com/apache/kafka/pull/18009
The controller must add all extant brokers to BrokerHeartbeatTracker when activating. Otherwise, we could end up in a situation where a broker fails exactly as a controller failover occurs, and we never fence it.
Also, fix a bug where the slf4j logger object in PeriodicTaskControlManager was initialized as though it belonged to OffsetControlManager.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18138: The controller must add all extant brokers to BrokerHeartbeatTracker when activating [kafka]
Posted by "splett2 (via GitHub)" <gi...@apache.org>.
splett2 commented on code in PR #18009:
URL: https://github.com/apache/kafka/pull/18009#discussion_r1866766986
##########
metadata/src/main/java/org/apache/kafka/controller/ClusterControlManager.java:
##########
@@ -306,8 +306,11 @@ ReplicaPlacer replicaPlacer() {
*/
public void activate() {
heartbeatManager = new BrokerHeartbeatManager(logContext, time, sessionTimeoutNs);
+ long nowNs = time.nanoseconds();
for (BrokerRegistration registration : brokerRegistrations.values()) {
heartbeatManager.register(registration.id(), registration.fenced());
+ heartbeatManager.tracker().updateContactTime(
+ new BrokerIdAndEpoch(registration.id(), registration.epoch()), nowNs);
Review Comment:
sounds good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18138: The controller must add all extant brokers to BrokerHeartbeatTracker when activating [kafka]
Posted by "splett2 (via GitHub)" <gi...@apache.org>.
splett2 commented on code in PR #18009:
URL: https://github.com/apache/kafka/pull/18009#discussion_r1866734187
##########
metadata/src/main/java/org/apache/kafka/controller/ClusterControlManager.java:
##########
@@ -306,8 +306,11 @@ ReplicaPlacer replicaPlacer() {
*/
public void activate() {
heartbeatManager = new BrokerHeartbeatManager(logContext, time, sessionTimeoutNs);
+ long nowNs = time.nanoseconds();
for (BrokerRegistration registration : brokerRegistrations.values()) {
heartbeatManager.register(registration.id(), registration.fenced());
+ heartbeatManager.tracker().updateContactTime(
+ new BrokerIdAndEpoch(registration.id(), registration.epoch()), nowNs);
Review Comment:
nit: may want to consider only adding unfenced brokers?
adding fenced brokers to the tracker may delay fencing a newly failed broker by whatever is the session timeout.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18138: The controller must add all extant brokers to BrokerHeartbeatTracker when activating [kafka]
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #18009:
URL: https://github.com/apache/kafka/pull/18009#discussion_r1866761231
##########
metadata/src/main/java/org/apache/kafka/controller/ClusterControlManager.java:
##########
@@ -306,8 +306,11 @@ ReplicaPlacer replicaPlacer() {
*/
public void activate() {
heartbeatManager = new BrokerHeartbeatManager(logContext, time, sessionTimeoutNs);
+ long nowNs = time.nanoseconds();
for (BrokerRegistration registration : brokerRegistrations.values()) {
heartbeatManager.register(registration.id(), registration.fenced());
+ heartbeatManager.tracker().updateContactTime(
+ new BrokerIdAndEpoch(registration.id(), registration.epoch()), nowNs);
Review Comment:
I think it's OK to add them here just for consistency with the `processBrokerHeartbeat` code, which also adds them. It shouldn't delay anything; the tracker just manages last contact times.
In the case where both a fenced and an unfenced broker had timed out, we would remove the fenced one from the tracker, do nothing, and then reschedule `maybeFenceOneStaleBroker` immediately as the next controllerr event. So there is no delay really.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18138: The controller must add all extant brokers to BrokerHeartbeatTracker when activating [kafka]
Posted by "mumrah (via GitHub)" <gi...@apache.org>.
mumrah merged PR #18009:
URL: https://github.com/apache/kafka/pull/18009
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-18138: The controller must add all extant brokers to BrokerHeartbeatTracker when activating [kafka]
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #18009:
URL: https://github.com/apache/kafka/pull/18009#discussion_r1866761231
##########
metadata/src/main/java/org/apache/kafka/controller/ClusterControlManager.java:
##########
@@ -306,8 +306,11 @@ ReplicaPlacer replicaPlacer() {
*/
public void activate() {
heartbeatManager = new BrokerHeartbeatManager(logContext, time, sessionTimeoutNs);
+ long nowNs = time.nanoseconds();
for (BrokerRegistration registration : brokerRegistrations.values()) {
heartbeatManager.register(registration.id(), registration.fenced());
+ heartbeatManager.tracker().updateContactTime(
+ new BrokerIdAndEpoch(registration.id(), registration.epoch()), nowNs);
Review Comment:
I think it's OK to add them here just for consistency with the `processBrokerHeartbeat` code, which also adds them. It shouldn't delay anything; the tracker just manages last contact times.
In the case where both a fenced and an unfenced broker had timed out, the code in `maybeFenceOneStaleBroker` would remove the fenced one from the tracker, do nothing, and then reschedule `maybeFenceOneStaleBroker` immediately as the next controllerr event. So there is no delay really.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org