You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "kevin-wu24 (via GitHub)" <gi...@apache.org> on 2025/08/14 12:26:39 UTC
[PR] KAFKA-19605: Fix the busy loop occurring in the broker observer [kafka]
kevin-wu24 opened a new pull request, #20354:
URL: https://github.com/apache/kafka/pull/20354
The broker observer should not read update voter set timer value when polling.
--
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-19605: Fix the busy loop occurring in the broker observer [kafka]
Posted by "kevin-wu24 (via GitHub)" <gi...@apache.org>.
kevin-wu24 commented on code in PR #20354:
URL: https://github.com/apache/kafka/pull/20354#discussion_r2278894794
##########
raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java:
##########
@@ -3356,17 +3352,13 @@ private long pollFollowerAsObserver(FollowerState state, long currentTimeMs) {
} else {
sendResult = maybeSendAddVoterRequest(state, currentTimeMs);
}
- backoffMs = sendResult.timeToWaitMs();
if (sendResult.requestSent()) {
state.resetUpdateVoterSetPeriod(currentTimeMs);
}
+ return sendResult.timeToWaitMs();
Review Comment:
The observer does not need to consider the time left of the fetch timeout when calculating the backoff, because an observer cannot transition to prospective/candidate state. It must transition to follower state first.
--
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-19605: Fix the busy loop occurring in the broker observer [kafka]
Posted by "ahuang98 (via GitHub)" <gi...@apache.org>.
ahuang98 commented on code in PR #20354:
URL: https://github.com/apache/kafka/pull/20354#discussion_r2277415274
##########
raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java:
##########
@@ -3315,10 +3315,7 @@ private long pollFollowerAsVoter(FollowerState state, long currentTimeMs) {
return Math.min(
backoffMs,
- Math.min(
- state.remainingFetchTimeMs(currentTimeMs),
- state.remainingUpdateVoterSetPeriodMs(currentTimeMs)
- )
+ state.remainingFetchTimeMs(currentTimeMs)
Review Comment:
I see, the backoffMs accounts for the time to wait before processing the result of any updateVoteRequest
--
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-19605: Fix the busy loop occurring in the broker observer [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #20354:
URL: https://github.com/apache/kafka/pull/20354#issuecomment-3188461558
> I'm not super sure what a unit test would look like, since the backoff logic is not a correctness thing, but rather an efficiency/performance thing.
It seems to me that the busy loop is a performance issue, as it could lead to high CPU usage. I'm fine with adding a unit test in a follow-up, since the local test looks good. Otherwise, as soon as the broker starts running, my computer's fan spins up, which is a bit alarming.
--
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-19605: Fix the busy loop occurring in the broker observer [kafka]
Posted by "kevin-wu24 (via GitHub)" <gi...@apache.org>.
kevin-wu24 commented on code in PR #20354:
URL: https://github.com/apache/kafka/pull/20354#discussion_r2278894794
##########
raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java:
##########
@@ -3356,17 +3352,13 @@ private long pollFollowerAsObserver(FollowerState state, long currentTimeMs) {
} else {
sendResult = maybeSendAddVoterRequest(state, currentTimeMs);
}
- backoffMs = sendResult.timeToWaitMs();
if (sendResult.requestSent()) {
state.resetUpdateVoterSetPeriod(currentTimeMs);
}
+ return sendResult.timeToWaitMs();
Review Comment:
The observer does not need to consider the time left on the fetch timer when calculating the backoff, because an observer cannot transition to prospective/candidate state. It must transition to follower state first.
--
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-19605: Fix the busy loop occurring in the broker observer [kafka]
Posted by "kevin-wu24 (via GitHub)" <gi...@apache.org>.
kevin-wu24 commented on code in PR #20354:
URL: https://github.com/apache/kafka/pull/20354#discussion_r2278894794
##########
raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java:
##########
@@ -3356,17 +3352,13 @@ private long pollFollowerAsObserver(FollowerState state, long currentTimeMs) {
} else {
sendResult = maybeSendAddVoterRequest(state, currentTimeMs);
}
- backoffMs = sendResult.timeToWaitMs();
if (sendResult.requestSent()) {
state.resetUpdateVoterSetPeriod(currentTimeMs);
}
+ return sendResult.timeToWaitMs();
Review Comment:
The observer does not need to consider the time left of the fetch timeout when backing off, because an observer cannot transition to prospective/candidate state. It must transition to follower state first.
--
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-19605: Fix the busy loop occurring in the broker observer [kafka]
Posted by "kevin-wu24 (via GitHub)" <gi...@apache.org>.
kevin-wu24 commented on PR #20354:
URL: https://github.com/apache/kafka/pull/20354#issuecomment-3188995625
Hi @chia7712, @jsancio and I had a discussion offline about some of the "backoff" logic in `pollFollowerAsObserver` and `pollFollowerAsVoter`. It is partially related to the bug here, but mainly the problem is that the calculation of the return value for these methods is conceptually incorrect as it is now. Basically, the fetch timeout and update voter set timeout mean two different things conceptually, but the code treats them similarly.
- When the fetch timeout expires, it means the voter should transition states. This means the fetch timeout is something we should consider when calculating the backoff.
- The update voter set timeout just means that every X amount of time, the replica should try to send an Add/Remove/UpdateVoterRPC. The value of this timer is not something we should consider when calculating the backoff. If we successfully send one of these RPCs, we should wait for `sendResult.timeToWaitMs()`. If we don't successfully send one of these RPCs because there is a pending request, we should also wait for `sendResult.timeToWaitMs()`.
--
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-19605: Fix the busy loop occurring in the broker observer [kafka]
Posted by "kevin-wu24 (via GitHub)" <gi...@apache.org>.
kevin-wu24 commented on code in PR #20354:
URL: https://github.com/apache/kafka/pull/20354#discussion_r2278894794
##########
raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java:
##########
@@ -3356,17 +3352,13 @@ private long pollFollowerAsObserver(FollowerState state, long currentTimeMs) {
} else {
sendResult = maybeSendAddVoterRequest(state, currentTimeMs);
}
- backoffMs = sendResult.timeToWaitMs();
if (sendResult.requestSent()) {
state.resetUpdateVoterSetPeriod(currentTimeMs);
}
+ return sendResult.timeToWaitMs();
Review Comment:
The observer does not need to consider the time left on the fetch timer when calculating the backoff, because an observer cannot transition to prospective/candidate state. It must transition to follower state first.
> What happens if timeToWaitMs is larger than the fetch timeout?
If this is the case, the observer would have to wait timeToWaitMs anyways so its request manager doesn't have a pending request. Only then can it resume fetching/sending add/remove voter.
##########
raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java:
##########
@@ -3356,17 +3352,13 @@ private long pollFollowerAsObserver(FollowerState state, long currentTimeMs) {
} else {
sendResult = maybeSendAddVoterRequest(state, currentTimeMs);
}
- backoffMs = sendResult.timeToWaitMs();
if (sendResult.requestSent()) {
state.resetUpdateVoterSetPeriod(currentTimeMs);
}
+ return sendResult.timeToWaitMs();
Review Comment:
The observer does not need to consider the time left on the fetch timer when calculating the backoff, because an observer cannot transition to prospective/candidate state. It must transition to follower state first.
> What happens if timeToWaitMs is larger than the fetch timeout?
If this is the case, the observer would have to wait timeToWaitMs anyways so its request manager doesn't have a pending request. Only then can it resume fetching/sending add/remove voter.
--
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-19605: Fix the busy loop occurring in the broker observer [kafka]
Posted by "kevin-wu24 (via GitHub)" <gi...@apache.org>.
kevin-wu24 commented on code in PR #20354:
URL: https://github.com/apache/kafka/pull/20354#discussion_r2278894794
##########
raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java:
##########
@@ -3356,17 +3352,13 @@ private long pollFollowerAsObserver(FollowerState state, long currentTimeMs) {
} else {
sendResult = maybeSendAddVoterRequest(state, currentTimeMs);
}
- backoffMs = sendResult.timeToWaitMs();
if (sendResult.requestSent()) {
state.resetUpdateVoterSetPeriod(currentTimeMs);
}
+ return sendResult.timeToWaitMs();
Review Comment:
The observer does not need to consider the time left of the fetch timeout when calculating the back =off, because an observer cannot transition to prospective/candidate state. It must transition to follower state first.
--
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-19605: Fix the busy loop occurring in the broker observer [kafka]
Posted by "kevin-wu24 (via GitHub)" <gi...@apache.org>.
kevin-wu24 commented on PR #20354:
URL: https://github.com/apache/kafka/pull/20354#issuecomment-3189799132
@chia7712 can you re-trigger the CI? The Java 24 failure doesn't look related.
--
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-19605: Fix the busy loop occurring in the broker observer [kafka]
Posted by "jsancio (via GitHub)" <gi...@apache.org>.
jsancio commented on code in PR #20354:
URL: https://github.com/apache/kafka/pull/20354#discussion_r2276816343
##########
raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java:
##########
@@ -3363,10 +3366,14 @@ private long pollFollowerAsObserver(FollowerState state, long currentTimeMs) {
} else {
backoffMs = maybeSendFetchToBestNode(state, currentTimeMs);
}
- return Math.min(
- backoffMs,
- state.remainingUpdateVoterSetPeriodMs(currentTimeMs)
- );
+ if (shouldSendAddOrRemoveVoterRequest()) {
+ return Math.min(
+ backoffMs,
+ state.remainingUpdateVoterSetPeriodMs(currentTimeMs)
+ );
+ } else {
+ return backoffMs;
+ }
Review Comment:
Can you change the implementation so that `shouldSendAddOrRemoveVoterRequest` is only evaluated once?
You can also change the back off computation to something like:
```java
return Math.min(
backoffMs,
shouldSendAddOrRemoveVoterRequest ?
state.remainingUpdateVoterSetPeriodMs(currentTimeMs) :
Integer.MAX_VALUE
);
```
--
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-19605: Fix the busy loop occurring in the broker observer [kafka]
Posted by "kevin-wu24 (via GitHub)" <gi...@apache.org>.
kevin-wu24 commented on PR #20354:
URL: https://github.com/apache/kafka/pull/20354#issuecomment-3188378625
> Is it possible to add a unit test for it?
I'm not super sure what a unit test would look like, since the backoff logic is not a correctness thing, but rather an efficiency/performance thing.
--
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-19605: Fix the busy loop occurring in the broker observer [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #20354:
URL: https://github.com/apache/kafka/pull/20354#discussion_r2278892176
##########
raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java:
##########
@@ -3356,17 +3352,13 @@ private long pollFollowerAsObserver(FollowerState state, long currentTimeMs) {
} else {
sendResult = maybeSendAddVoterRequest(state, currentTimeMs);
}
- backoffMs = sendResult.timeToWaitMs();
if (sendResult.requestSent()) {
state.resetUpdateVoterSetPeriod(currentTimeMs);
}
+ return sendResult.timeToWaitMs();
Review Comment:
What happens if `timeToWaitMs` is larger than the fetch timeout? Could the observer miss a fetch request?
--
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-19605; Fix the busy loop occurring in kraft client observers [kafka]
Posted by "jsancio (via GitHub)" <gi...@apache.org>.
jsancio merged PR #20354:
URL: https://github.com/apache/kafka/pull/20354
--
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