KAFKA-4840 : BufferPool errors can cause buffer pool to go into a bad state by smccauliff · Pull Request #2659 · apache/kafka

smccauliff · 2017-03-08T22:51:33Z

No description provided.

ijuma · 2017-03-08T22:56:52Z

We don't need the BUFFERPOOL prefix here since it's already defined in the BufferPool class. Thanks for the PR, will hopefully review it soon.

Thanks for the feedback. I've removed this prefix.

asfbot · 2017-03-08T23:49:35Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2089/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-03-08T23:52:13Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2087/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-03-09T00:23:48Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2086/
Test PASSed (JDK 7 and Scala 2.10).

asfbot · 2017-03-13T17:47:47Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2147/
Test FAILed (JDK 8 and Scala 2.11).

asfbot · 2017-03-13T17:49:11Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2144/
Test PASSed (JDK 7 and Scala 2.10).

asfbot · 2017-03-13T18:18:02Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2145/
Test FAILed (JDK 8 and Scala 2.12).

Readability.

… something in the free list.

Always signal waiters when allocation is complete. Remove BUFFERPOOL prefix from constant.

lindong28 · 2017-03-15T21:10:53Z

I am not sure it is useful to have a static variable for this string since it is used only once. It is also different from the code style in Sender.SenderMetrics which assigns sensor name directly. Maybe we can preserve the existing style for simplicity and code style consistence.

There is a mock that refers to this variable otherwise I would not have bothered to put this into a static final variable.

lindong28 · 2017-03-15T21:53:19Z

Since we only release lock in the finally here, do we still need to check lock.isHeldByCurrentThread?

Probably not. The original code was unlocking in different places so this was needed. I will remove this.

lindong28 · 2017-03-15T21:59:41Z

The name restoreAvailableMemoryOnFailure is a bit weird because we should always restore available memory on failure. Maybe we can name it hasError and set it to false right before return buffer.

Sounds good.

lindong28 · 2017-03-15T22:00:41Z

We can probably remove this line restoreAvailableMemoryOnFailure = false.

lindong28 · 2017-03-15T22:01:40Z

Should we also remove this condition variable within a finally block?

lindong28 · 2017-03-15T22:02:14Z

Would it be possible and simpler to merge the two finally into one?

Not without additional complications. One finally deals with cases caused by waiting for memory to become available the other deals with cases causes by just allocating memory.

I think there is actually a reasonable way to merge the two finally into one. The idea is that the code in the first finally block doesn't throw exception and is no-op if this.availableMemory + freeListSize >= size. This is no-op if you change it to the following:

if (hasError) this.availableMemory += accumulated; if (moreMemory != null) this.waiters.remove(moreMemory);

Don't check to see if lock is held before unlocking, since there is only one place where the lock is unlocked.

smccauliff · 2017-03-27T06:37:14Z

@lindong28 Can you check this again? Thanks.

asfbot · 2017-03-27T07:21:51Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2400/
Test PASSed (JDK 7 and Scala 2.10).

asfbot · 2017-03-27T07:22:11Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2404/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-03-27T07:53:36Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2400/
Test PASSed (JDK 8 and Scala 2.12).

lindong28 · 2017-03-27T16:55:53Z

@smccauliff I think we can further simplify the code by merging the two finally into one. But it is OK to me if you don't want to do that. LGTM. Ping @becketqin for review.

becketqin · 2017-04-05T23:28:34Z

                freeUp(size);
                this.availableMemory -= size;
-                lock.unlock();
                return allocateByteBuffer(size);


If an OOM is thrown from here, it seems the available memory would be already decremented and not added back.

…lable.

asfbot · 2017-04-06T17:43:47Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2788/
Test PASSed (JDK 7 and Scala 2.10).

asfbot · 2017-04-06T17:46:28Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2788/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-04-06T17:47:04Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2792/
Test PASSed (JDK 8 and Scala 2.11).

becketqin · 2017-04-13T03:54:00Z

+            // over for them
+            if (!(this.availableMemory == 0 && this.free.isEmpty()) && !this.waiters.isEmpty())
+                this.waiters.peekFirst().signal();
+            lock.unlock();


It seems findBugs is not happy with this finally block although I think it should be fine. Can we have a try finally block here to ensure lock.unlock() is executed so that findBugs is happy?

becketqin · 2017-04-13T03:55:13Z

@smccauliff Thanks for the update. LGTM. findBugs was complaining about one of the finally block. Can we change it to make findBugs happy?

asfbot · 2017-04-13T03:59:14Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2922/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-04-13T03:59:16Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2926/
Test FAILed (JDK 8 and Scala 2.11).

asfbot · 2017-04-13T03:59:40Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2921/
Test FAILed (JDK 7 and Scala 2.10).

asfbot · 2017-04-25T13:15:58Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/3162/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-04-25T13:15:58Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/3166/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-04-25T14:15:59Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/3159/
Test PASSed (JDK 7 and Scala 2.10).

becketqin · 2017-04-25T17:31:53Z

Thanks for updating the patch. Merged to trunk.

ijuma · 2017-04-25T22:50:30Z

                // we have enough unallocated or pooled memory to immediately
                // satisfy the request
                freeUp(size);
+                ByteBuffer allocatedBuffer = allocateByteBuffer(size);


I think it was intentional that we didn't allocate memory with the lock held. It seems like this optimisation was lost?

I'll see if there is some clean way to restore that optimization, but if there is OOM on buffer allocation then the lock needs to be acquired and the allocation freed once more. Seems pretty ugly.

Yeah, it's not pretty. However, reducing the concurrency of BufferPool in the common path is not desireable. We definitely need to handle OOMs correctly, but they are relatively rare and it's OK if that path is slower.

Good point. I think it is still worth keeping the optimization, although typically the the producer will only allocate poolable batch size, so the actual memory allocation should not happen very often.

ijuma reviewed Mar 8, 2017

View reviewed changes

smccauliff added 6 commits March 15, 2017 09:06

Remove more memory condition variable on throwable or sensor exception.

073bea4

Test sensor report exception.

d9fbeaa

Fix checkstyle violations.

77ada00

Readability.

Restore avaialble memory count on exception.

140c80a

Next waiter can check the new available memory, we don't need to have…

0a0f677

… something in the free list.

Only unlock and signal waiters in one place.

93d6b16

Always signal waiters when allocation is complete. Remove BUFFERPOOL prefix from constant.

lindong28 reviewed Mar 15, 2017

View reviewed changes

Consolidate the cleanup of memory allocation into fewer lines of code.

45b2493

smccauliff force-pushed the kafka-4840 branch from 620727e to 45b2493 Compare March 27, 2017 06:25

Rename variable.

b6d7106

Don't check to see if lock is held before unlocking, since there is only one place where the lock is unlocked.

becketqin reviewed Apr 5, 2017

View reviewed changes

OOM can cause buffer pool to loose byte when it has memory count avai…

7449124

…lable.

becketqin reviewed Apr 13, 2017

View reviewed changes

Another finally otherwise findbugs complains.

4d77cfc

asfgit closed this in c43eb85 Apr 25, 2017

ijuma reviewed Apr 25, 2017

View reviewed changes

colinhicks mentioned this pull request Oct 15, 2018

MINOR: Less restrictive assertion in flaky BufferPool test #5799

Merged

3 tasks

Conversation

smccauliff commented Mar 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfbot commented Mar 8, 2017

Uh oh!

asfbot commented Mar 8, 2017

Uh oh!

asfbot commented Mar 9, 2017

Uh oh!

asfbot commented Mar 13, 2017

Uh oh!

asfbot commented Mar 13, 2017

Uh oh!

asfbot commented Mar 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smccauliff commented Mar 27, 2017

Uh oh!

asfbot commented Mar 27, 2017

Uh oh!

asfbot commented Mar 27, 2017

Uh oh!

asfbot commented Mar 27, 2017

Uh oh!

lindong28 commented Mar 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfbot commented Apr 6, 2017

Uh oh!

asfbot commented Apr 6, 2017

Uh oh!

asfbot commented Apr 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

becketqin commented Apr 13, 2017

Uh oh!

asfbot commented Apr 13, 2017

Uh oh!

asfbot commented Apr 13, 2017

Uh oh!

asfbot commented Apr 13, 2017

Uh oh!

asfbot commented Apr 25, 2017

Uh oh!

asfbot commented Apr 25, 2017

Uh oh!

asfbot commented Apr 25, 2017

Uh oh!

becketqin commented Apr 25, 2017