From 5a6031373d78a5de9d48e75d98b92de449576154 Mon Sep 17 00:00:00 2001 From: Colin Hicks Date: Fri, 12 Oct 2018 13:57:45 -0400 Subject: [PATCH 1/4] MINOR: remove non-deterministic assertion in flaky BufferPool test. --- .../producer/internals/BufferPoolTest.java | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/clients/src/test/java/org/apache/kafka/clients/producer/internals/BufferPoolTest.java b/clients/src/test/java/org/apache/kafka/clients/producer/internals/BufferPoolTest.java index ce74eb1f0f1db..adc492aa1f59b 100644 --- a/clients/src/test/java/org/apache/kafka/clients/producer/internals/BufferPoolTest.java +++ b/clients/src/test/java/org/apache/kafka/clients/producer/internals/BufferPoolTest.java @@ -154,7 +154,7 @@ 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 { @@ -162,10 +162,10 @@ public void testBlockTimeout() throws Exception { 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(); @@ -175,9 +175,9 @@ public void testBlockTimeout() throws Exception { } catch (TimeoutException e) { // this is good } - assertTrue("available memory" + pool.availableMemory(), pool.availableMemory() >= 9 && pool.availableMemory() <= 10); - long endTimeMs = Time.SYSTEM.milliseconds(); - assertTrue("Allocation should finish not much later than maxBlockTimeMs", endTimeMs - beginTimeMs < maxBlockTimeMs + 1000); + long durationMs = Time.SYSTEM.milliseconds() - beginTimeMs; + assertTrue("TimeoutException should not throw before maxBlockTimeMs", durationMs >= maxBlockTimeMs); + assertTrue("TimeoutException should throw soon after maxBlockTimeMs", durationMs < maxBlockTimeMs + 100); } /** @@ -193,7 +193,8 @@ public void testCleanupMemoryAvailabilityWaiterOnBlockTimeout() throws Exception } catch (TimeoutException e) { // this is good } - assertTrue(pool.queued() == 0); + assertEquals(0, pool.queued()); + assertEquals(1, pool.availableMemory()); } /** From 4847884f15af5fbfd67a3522f05f05ad73614eb3 Mon Sep 17 00:00:00 2001 From: Colin Hicks Date: Sat, 20 Oct 2018 14:24:55 -0400 Subject: [PATCH 2/4] Address code review feedback. --- .../kafka/clients/producer/internals/BufferPoolTest.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/clients/src/test/java/org/apache/kafka/clients/producer/internals/BufferPoolTest.java b/clients/src/test/java/org/apache/kafka/clients/producer/internals/BufferPoolTest.java index adc492aa1f59b..e14ca252137d6 100644 --- a/clients/src/test/java/org/apache/kafka/clients/producer/internals/BufferPoolTest.java +++ b/clients/src/test/java/org/apache/kafka/clients/producer/internals/BufferPoolTest.java @@ -175,9 +175,11 @@ public void testBlockTimeout() throws Exception { } catch (TimeoutException e) { // this is good } + // Thread scheduling sometimes means that no deallocation happens 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 + 100); + assertTrue("TimeoutException should throw soon after maxBlockTimeMs", durationMs < maxBlockTimeMs + 1000); } /** From 7bb618d06a41c545c0ef8e7a29600cbd3df24180 Mon Sep 17 00:00:00 2001 From: Colin Hicks Date: Sat, 20 Oct 2018 14:28:45 -0400 Subject: [PATCH 3/4] Correct spacing in assertion message. --- .../apache/kafka/clients/producer/internals/BufferPoolTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clients/src/test/java/org/apache/kafka/clients/producer/internals/BufferPoolTest.java b/clients/src/test/java/org/apache/kafka/clients/producer/internals/BufferPoolTest.java index e14ca252137d6..57f47d6053f3f 100644 --- a/clients/src/test/java/org/apache/kafka/clients/producer/internals/BufferPoolTest.java +++ b/clients/src/test/java/org/apache/kafka/clients/producer/internals/BufferPoolTest.java @@ -176,7 +176,7 @@ public void testBlockTimeout() throws Exception { // this is good } // Thread scheduling sometimes means that no deallocation happens by this point - assertTrue("available memory" + pool.availableMemory(), pool.availableMemory() >= 8 && pool.availableMemory() <= 10); + 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); From cc552478c8d71a38a016f88f1c4f6b9070492026 Mon Sep 17 00:00:00 2001 From: Colin Hicks Date: Sat, 20 Oct 2018 14:31:26 -0400 Subject: [PATCH 4/4] Clarify deallocation timing comment. --- .../apache/kafka/clients/producer/internals/BufferPoolTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clients/src/test/java/org/apache/kafka/clients/producer/internals/BufferPoolTest.java b/clients/src/test/java/org/apache/kafka/clients/producer/internals/BufferPoolTest.java index 57f47d6053f3f..8e44fa18146d4 100644 --- a/clients/src/test/java/org/apache/kafka/clients/producer/internals/BufferPoolTest.java +++ b/clients/src/test/java/org/apache/kafka/clients/producer/internals/BufferPoolTest.java @@ -175,7 +175,7 @@ public void testBlockTimeout() throws Exception { } catch (TimeoutException e) { // this is good } - // Thread scheduling sometimes means that no deallocation happens by this point + // 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);