KAFKA-17661: Fix flaky BufferPoolTest.testBlockTimeout

[chenyulin0719]

It's regarding KAFKA-17661.

The test relies on 3 asynchronous threads being triggered in parallel with the test thread[1]. However, there is no guarantee of parallelism in test environment. The 25 ms delay(10 ms maxBlockTimeMs) is obviously unreliable in the test environment.

To address the racing between asynchronous threads, removing the third delayed deallocation to ensure pool.allocate(10, maxBlockTimeMs) always hits the timeout.

[1]

kafka/clients/src/test/java/org/apache/kafka/clients/producer/internals/BufferPoolTest.java

Lines 175 to 179 in 4036081

	// The first two buffers will be de-allocated within maxBlockTimeMs since the most recent allocation
	delayedDeallocate(pool, buffer1, maxBlockTimeMs / 2);
	delayedDeallocate(pool, buffer2, maxBlockTimeMs);
	// The third buffer will be de-allocated after maxBlockTimeMs since the most recent allocation
	delayedDeallocate(pool, buffer3, maxBlockTimeMs / 2 * 5);

Committer Checklist (excluded from commit message)

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

[chenyulin0719]

Another possible solution is to remove the asynchronous delayedDeallocate() in testBlockTimeout.
But I prefer to keep the more complexity test since there are no similar tests existing in BufferPoolTest.

[chia7712]

Another possible solution is to remove the asynchronous delayedDeallocate() in testBlockTimeout.

In fact, this solution looks good to me. We can have a follow-up to add multi-threaded tests for deallocate and allocate, but I don't think this test case requires asynchronous operations.

[chenyulin0719]

@chia7712 That make sense to me. I will rewrite this PR.

[chenyulin0719]

Hi @chia7712,

After rethink about it, I believe we can resolve the racing issue while still keeping the multi-threading test logic by removing the third delayed deallocation.

The root casue of racing is all the three delayed deallocation threads occur before the test thread. In this case pool.allocate(10, maxBlockTimeMs in test thread will get memory immediately.
So we can remove the third deallocation to make sure the pool.allocate(10, maxBlockTimeMs) will never be satisfied.

The following test target can be kept in the test:

1. pool.allocate(10, maxBlockTimeMs) waits for maxBlockTimeMs for more memory (Link)
2. pool.allocate(10, maxBlockTimeMs) won't returned early when receiving a notification from other thread. (Link)

Below is the detailed explanation for the new test:

• [test thread] allocate buffer 1 with 1 memory
• [test thread] allocate buffer 2 with 1 memory
• [test thread] allocate buffer 3 with 1 memory
• [test thread] request buffer with 10 memory, await for more memroy
• [delayed thread] At 5 ms, deallocate buffer 1, signal test thread
• [test thread] Awaken at 5 ms, await again for more memory
• [test thread] pool.allocate(10, maxBlockTimeMs timeout at 10 ms -> Can run assertions after this point.
• [delayed thread] At 10 ms, deallocate buffer 2

No matter when the delayed thread occurs, the timeout in the test thread always hits.
(pool.allocate(10, maxBlockTimeMs) never get enough memory)

Just updated the PR to this version. The test passes 500 loops in my local env. And also passes after adding delay to test thread.

[chia7712]

(pool.allocate(10, maxBlockTimeMs) never get enough memory)

What if the previous two threads haven’t allocated before this allocation? Would it still encounter a buffer exception?

[chenyulin0719]

@chia7712
The asynchronous thread is for pool.deallocate(), not for allocation.

What if the previous two threads haven’t allocated before this allocation?

So it won't happen, the first three pool.allocate(1, maxBlockTimeMs) calls are run in the test thread, they are synchronous.

[chia7712]

The asynchronous thread is for pool.deallocate(), not for allocation.

Sorry for the typo. My point is that deallocate is a no-op in this test, as the case will pass even if those threads haven't deallocated. If you prefer the solution of removing the threads, you should remove all deallocation threads

[chenyulin0719]

My point is that deallocate is a no-op in this test, as the case will pass even if those threads haven't deallocated.

You're right, removing the async deallocation doesn't change the test result.
The reason I decided to keep the async deallocation is that the deallocate threads are intended to validate this correct behavior under multi-threading scenario:

• When a block allocate() is signaled by other thread's deallocate() call (BufferPool.java#L273-L275), it still re-enter the the while loop until hits the block timeout. (BufferPool.java#L148-L153)

I won't say the deallcoate is a no-op in this test, but the orignal test name is not accurate.
I can split the test into 2 tests as you suggested previously. Split the testBlockTimeout() into below tests:

1. testBlockTimeout()
2. testAsyncDeallocateNotBreakBlockTimeout()

@chia7712 WDYT?

[chia7712]

When a block allocate() is signaled by other thread's deallocate() call (BufferPool.java#L273-L275), it still re-enter the the while loop until hits the block timeout. (BufferPool.java#L148-L153)
I won't say the deallcoate is a no-op in this test, but the orignal test name is not accurate.

I don't think this test case has actually tested this behavior, which is why I believe the deallocate is a no-op, as the related behavior isn't asserted in the test.

I can split the test into 2 tests as you suggested previously. Split the testBlockTimeout() into below tests:

Perhaps this PR can simplify the test case to address the flakiness, and then you can file a follow-up to add more test cases.

[chenyulin0719]

OK, I just update this PR to simplify the test case.
We can create follow-up to discuss whether we should bring the multi-threading test back.

[mumrah]

@chenyulin0719 fyi, there was a build issue on trunk. I have fixed the issue and updated your branch with the fix.

[chenyulin0719]

@mumrah Thank you for letting me know. :)

[chia7712]

LGTM

@chenyulin0719 Please file a jira as follow-up to add more test cases