KAFKA-6948 - Change comparison to avoid overflow inconsistencies

[thisthat]

Change timestamp comparison following what the Java documentation recommends to help preventing such errors.

@guozhangwang I create the new PR as requested 😄

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[guozhangwang]

We've added a remainingTimeAtLeastZero function in multiple classes of consumer, maybe we can use that function?

[thisthat]

I think it is better to use it in line 575 to protect the call to ensureCoordinatorReady but not for the if guard due to the strict comparator. If it is correct to write

now - endTimeMs <= 0

then I can replace it with

remainingTimeAtLeastZero(now, endTimeMs) == 0

[guozhangwang]

Ack, makes sense.

[guozhangwang]

EDIT: thinking about this again, I think we need to consider two cases separately

1. for a parameter passed into a function call like poll() higher in the call trace like line 575 below, we need to make sure it is not a negative value, and hence it's better to use remainingTimeAtLeastZero.

2. for a condition comparing two values, we need to first of all make sure none of the comparing values may overflow, for example in KerberosLogin.java below if now + minTimeBeforeRelogin already overflows, then expiry - (now + minTimeBeforeRelogin) < 0 is not safe either. So I think a better solution would be, for any comparing values we makes sure they are directly from timer than as a result of sum, before going on to the condition check: for example in NetworkClientUtils.java above, the comparing expiryTime is a result of startTime + timeoutMs, instead, we should refactor it as:

long startTime = time.milliseconds();

// some calls that may take time

long attemptStartTime = time.milliseconds();

while (!client.isReady(node, attemptStartTime) && attemptStartTime - startTime < timeoutMs)

WDYT?

[thisthat]

For the first point I'm totally agreed with you.
Regarding the second instead, the inequation expiry - (now + minTimeBeforeRelogin) < 0 is still safe also if now + minTimeBeforeRelogin already overflowed. The critical bit with a timestamp comparison is to compare always with a constant/safe value.
Due to the finite arithmetic of computers, the safest way to compare timestamp variables is to perform operations on them only on one side of the inequation. I created a tool that automatically identifies the critical program points and performs these changes. Therefore, I use 0 as safe value. However, your suggestion of comparing values directly from timers such as attemptStartTime - startTime < timeoutMs is even better because it is more natural and readable.

If you agree, I would go though the code again and apply the changes according to the two cases.

[guozhangwang]

Sounds good, thanks.

[hachikuji]

For reference, #5087 would allows us to consolidate these checks and avoid all the inconsistent usage.

[guozhangwang]

Thanks for pointing out @hachikuji , is this PR going to be subsumed by #5087 ?

[thisthat]

I checked the changes in #5087 and, currently, there is an overlap only in the ConsumerCoordinator class.
I think the solution of @hachikuji that centralizes the handling of time operations will reduce the probability of introducing errors.
We may think to gradually replace the time comparisons with the Timer class provided in #5087

[guozhangwang]

The current PR LGTM. @hachikuji could you make a second pass before merging?

[guozhangwang]

cc @hachikuji again.

[ableegoldman]

A number of these fixes are no longer relevant since switching to Timers (I guess that was #10537 ), the remaining ones I just tacked onto this PR addressing a different overflow bug. So I think we can close this in favor of #11111