Skip to content

MINOR: Less restrictive assertion in flaky BufferPool test #5799

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ijuma merged 4 commits into from
Oct 21, 2018
Merged

MINOR: Less restrictive assertion in flaky BufferPool test #5799

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5a60313
MINOR: remove non-deterministic assertion in flaky BufferPool test.
colinhicks Oct 12, 2018
4847884
Address code review feedback.
colinhicks Oct 20, 2018
7bb618d
Correct spacing in assertion message.
colinhicks Oct 20, 2018
cc55247
Clarify deallocation timing comment.
colinhicks Oct 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions clients/src/test/java/org/apache/kafka/clients/producer/internals/BufferPoolTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,18 +154,18 @@ public void run() {

/**
* Test if Timeout exception is thrown when there is not enough memory to allocate and the elapsed time is greater than the max specified block time.
* And verify that the allocation should finish soon after the maxBlockTimeMs.
* And verify that the allocation attempt finishes soon after the maxBlockTimeMs.
*/
@Test
public void testBlockTimeout() throws Exception {
BufferPool pool = new BufferPool(10, 1, metrics, Time.SYSTEM, metricGroup);
ByteBuffer buffer1 = pool.allocate(1, maxBlockTimeMs);
ByteBuffer buffer2 = pool.allocate(1, maxBlockTimeMs);
ByteBuffer buffer3 = pool.allocate(1, maxBlockTimeMs);
// First two buffers will be de-allocated within maxBlockTimeMs since the most recent de-allocation
// 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 de-allocation
// The third buffer will be de-allocated after maxBlockTimeMs since the most recent allocation
delayedDeallocate(pool, buffer3, maxBlockTimeMs / 2 * 5);

long beginTimeMs = Time.SYSTEM.milliseconds();
Expand All @@ -175,9 +175,11 @@ public void testBlockTimeout() throws Exception {
} catch (TimeoutException e) {
// this is good
}
assertTrue("available memory" + pool.availableMemory(), pool.availableMemory() >= 9 && pool.availableMemory() <= 10);

@ijuma ijuma Oct 20, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of removing this, we should probably just say assert that it can be from 8 to 10 with a comment saying that thread scheduling sometimes means that no deallocation happens by the time we get here.

long endTimeMs = Time.SYSTEM.milliseconds();
assertTrue("Allocation should finish not much later than maxBlockTimeMs", endTimeMs - beginTimeMs < maxBlockTimeMs + 1000);
// Thread scheduling sometimes means that deallocation varies by this point
assertTrue("available memory " + pool.availableMemory(), pool.availableMemory() >= 8 && pool.availableMemory() <= 10);
long durationMs = Time.SYSTEM.milliseconds() - beginTimeMs;
assertTrue("TimeoutException should not throw before maxBlockTimeMs", durationMs >= maxBlockTimeMs);
assertTrue("TimeoutException should throw soon after maxBlockTimeMs", durationMs < maxBlockTimeMs + 1000);
}

/**
Expand All @@ -193,7 +195,8 @@ public void testCleanupMemoryAvailabilityWaiterOnBlockTimeout() throws Exception
} catch (TimeoutException e) {
// this is good
}
assertTrue(pool.queued() == 0);
assertEquals(0, pool.queued());
assertEquals(1, pool.availableMemory());
}

/**
Expand Down