KAFKA-19605; Fix the busy loop occurring in kraft client observers

[kevin-wu24]

The broker observer should not read update voter set timer value when
polling to determine its backoff, since brokers cannot auto-join the
KRaft voter set. If auto-join or kraft.version=1 is not supported,
controller observers should not read this timer either when polling.

The updateVoterSetPeriodMs timer is not something that should be
considered when calculating the backoff returned by polling, since this
timer does not represent the same thing as the fetchTimeMs timer.

Reviewers: Chia-Ping Tsai chia7712@gmail.com, José Armando García
Sancio jsancio@apache.org, Alyssa Huang ahuang@confluent.io,
Kuan-Po Tseng brandboat@gmail.com

[chia7712]

@kevin-wu24 thanks for this fix. I ran the patch locally, and the CPU usage has improved. Is it possible to add a unit test for it?

[kevin-wu24]

@chia7712 thanks for pointing out the issue.

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.

[chia7712]

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.

[jsancio]

Thanks for the fix @kevin-wu24. Just a minor coding comment.

[jsancio]

Can you change the implementation so that shouldSendAddOrRemoveVoterRequest is only evaluated once?

You can also change the back off computation to something like:

    return Math.min(
        backoffMs,
        shouldSendAddOrRemoveVoterRequest ?
            state.remainingUpdateVoterSetPeriodMs(currentTimeMs) :
            Integer.MAX_VALUE
    );

[kevin-wu24]

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(). The value of this timer does not impact how long we should back off for, only the in-flight request's lifetime.

[brandboat]

Before this patch:

After:

Profiling around 40 seconds, and the performance improvement is significant, thanks for the fix!

[brandboat]

BTW, now FollowerState#remainingUpdateVoterSetPeriodMs is an unused method, could we remove it?

[jsancio]

Thanks for the fix. LGTM.

[ahuang98]

I see, the backoffMs accounts for the time to wait before processing the result of any updateVoteRequest

[ahuang98]

thanks for the improvements!

[kevin-wu24]

@chia7712 can you re-trigger the CI? The Java 24 failure doesn't look related.

[chia7712]

What happens if timeToWaitMs is larger than the fetch timeout? Could the observer miss a fetch request?

[kevin-wu24]

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.