Skip to content

KAFKA-5239: Producer buffer pool allocates memory inside a lock#3053

Closed
smccauliff wants to merge 4 commits intoapache:trunkfrom
smccauliff:kafka-5239
Closed

KAFKA-5239: Producer buffer pool allocates memory inside a lock#3053
smccauliff wants to merge 4 commits intoapache:trunkfrom
smccauliff:kafka-5239

Conversation

@smccauliff
Copy link
Copy Markdown
Contributor

Move byte buffer allocation out of lock.
Add unit test for restoring count when OOM is thrown from byte buffer allocation.

Add unit test for restoring count when OOM is thrown from byte buffer allocation.
i
@asfbot
Copy link
Copy Markdown

asfbot commented May 15, 2017

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

@asfbot
Copy link
Copy Markdown

asfbot commented May 15, 2017

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

@asfbot
Copy link
Copy Markdown

asfbot commented May 15, 2017

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

@asfbot
Copy link
Copy Markdown

asfbot commented May 15, 2017

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

private final Deque<Condition> waiters;
/** This memory is accounted for separately from the poolable buffers in free. */
/** Poolable + non-poolable memory. */
private long availableMemory;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we change the definition of the available memory here, we may have to change a few other places as well. e.g. deallocate(), unallocatedMemory(), etc.

Copy link
Copy Markdown
Contributor Author

@smccauliff smccauliff Jun 3, 2017

Choose a reason for hiding this comment

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

I added the original comment; it is not correct.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Lines 234-236 confused me. But I see that freeUp() will potentially add the count of bytes back into availableMemory.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation. It seems the comment were still not updated? Have you pushed the change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

/** Poolable + non-poolable memory. */ is the correct comment. No?

@asfbot
Copy link
Copy Markdown

asfbot commented Jun 5, 2017

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

@asfbot
Copy link
Copy Markdown

asfbot commented Jun 5, 2017

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

@becketqin
Copy link
Copy Markdown
Contributor

Thanks for the patch. LGTM.

@asfgit asfgit closed this in 4b37918 Jun 6, 2017
@ijuma
Copy link
Copy Markdown
Member

ijuma commented Jun 6, 2017

Not sure if it's related, but the following test failed in Jenkins:

java.lang.AssertionError: available memory8
	at org.junit.Assert.fail(Assert.java:88)
	at org.junit.Assert.assertTrue(Assert.java:41)
	at org.apache.kafka.clients.producer.internals.BufferPoolTest.testBlockTimeout(BufferPoolTest.java:191)

https://builds.apache.org/blue/organizations/jenkins/kafka-trunk-jdk8/detail/kafka-trunk-jdk8/1660/tests

@becketqin
Copy link
Copy Markdown
Contributor

@ijuma I ran the tests a couple of times locally and cannot reproduce the issue. It seems an intermittent failure related to the delayedDeallocate. It does not look related to this patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants