Avoid using directBuffer directly #2158

sijie · 2019-09-06T09:33:26Z

Descriptions of the changes in this PR:

Motivation

We introduced a smart buffer allocator since 4.9.0. It falls back to allocate buffer
on heap when running out direct memory so that the bookie can run in a degraded mode
rather than crashes.

However if we use directBuffer directly, it bypasses the fallback mechanism. So
the bookie crashes when running out of direct memory.

Modifications

Use buffer instead of directBuffer and let the allocator decide what is the best
to use.

In order to uphold a high standard for quality for code contributions, Apache BookKeeper runs various precommit
checks for pull requests. A pull request can only be merged when it passes precommit checks. However running all
the precommit checks can take a long time, some trivial changes don't need to run all the precommit checks. You
can check following list to skip the tests that don't need to run for your pull request. Leave them unchecked if
you are not sure, committers will help you:

[skip bookkeeper-server bookie tests]: skip testing org.apache.bookkeeper.bookie in bookkeeper-server module.

[skip bookkeeper-server client tests]: skip testing org.apache.bookkeeper.client in bookkeeper-server module.

[skip bookkeeper-server replication tests]: skip testing org.apache.bookkeeper.replication in bookkeeper-server module.

[skip bookkeeper-server tls tests]: skip testing org.apache.bookkeeper.tls in bookkeeper-server module.

[skip bookkeeper-server remaining tests]: skip testing all other tests in bookkeeper-server module.

[skip integration tests]: skip docker based integration tests. if you make java code changes, you shouldn't skip integration tests.

[skip build java8]: skip build on java8. ONLY skip this when ONLY changing files under documentation under site.

[skip build java11]: skip build on java11. ONLY skip this when ONLY changing files under documentation under site.

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

If this PR is a BookKeeper Proposal (BP):

Make sure the PR title is formatted like:
<BP-#>: Description of bookkeeper proposal
e.g. BP-1: 64 bits ledger is support

Attach the master issue link in the description of this PR.

Attach the google doc link if the BP is written in Google Doc.

Otherwise:

Make sure the PR title is formatted like:
<Issue #>: Description of pull request
e.g. Issue 123: Description ...

Make sure tests pass via mvn clean apache-rat:check install spotbugs:check.

Replace <Issue #> in the title with the actual Issue number.

*Motivation* We introduced a smart buffer allocator since 4.9.0. It falls back to allocate buffer on heap when running out direct memory so that the bookie can run in a degraded mode rather than crashes. However if we use `directBuffer` directly, it bypasses the fallback mechanism. So the bookie crashes when running out of direct memory. *Modifications* Use `buffer` instead of `directBuffer` and let the allocator decide what is the best to use.

eolivelli

LGTM
Will merge as soon as CI passes

eolivelli · 2019-09-06T10:48:05Z

2019-09-06T09:38:45.945 [INFO] Starting audit...
[ERROR] /home/jenkins/jenkins-slave/workspace/bookkeeper_precommit_pullrequest_validation/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/WriteCache.java:27:8: Unused import: io.netty.buffer.Unpooled. [UnusedImports]
Audit done.
2019-09-06T09:38:45.945 [INFO] There is 1 error reported by Checkstyle 6.19 with buildtools/src/main/resources/bookkeeper/checkstyle.xml ruleset.

eolivelli · 2019-09-06T11:35:54Z

@diegosalvi this may be your problem of today :-)
Please take a look

diegosalvi · 2019-09-06T11:47:06Z

@eolivelli Unfortunately I think no. I've got many OOM due to newDirectBuffer invocation by netty itself as:

org.apache.bookkeeper.common.allocator.impl.ByteBufAllocatorImpl.newDirectBuffer(ByteBufAllocatorImpl.java:158)
at io.netty.buffer.AbstractByteBufAllocator.directBuffer(AbstractByteBufAllocator.java:187)

My guess in such cases newDirectBuffer "interface" method should route to buffer public buffer method and add a new private internalNewDirectBuffer to really instance buffers.. but I think is out of scope.

Stacktrace

19-09-06-10-58-40       io.netty.util.internal.OutOfDirectMemoryError: failed to allocate 16777216 byte(s) of direct memory (used: 1056964887, max: 1073741824)
io.netty.util.internal.OutOfDirectMemoryError: failed to allocate 16777216 byte(s) of direct memory (used: 1056964887, max: 1073741824)
        at io.netty.util.internal.PlatformDependent.incrementMemoryCounter(PlatformDependent.java:656)
        at io.netty.util.internal.PlatformDependent.allocateDirectNoCleaner(PlatformDependent.java:611)
        at io.netty.buffer.PoolArena$DirectArena.allocateDirect(PoolArena.java:768)
        at io.netty.buffer.PoolArena$DirectArena.newChunk(PoolArena.java:744)
        at io.netty.buffer.PoolArena.allocateNormal(PoolArena.java:245)
        at io.netty.buffer.PoolArena.allocate(PoolArena.java:215)
        at io.netty.buffer.PoolArena.allocate(PoolArena.java:147)
        at io.netty.buffer.PooledByteBufAllocator.newDirectBuffer(PooledByteBufAllocator.java:327)
        at io.netty.buffer.AbstractByteBufAllocator.directBuffer(AbstractByteBufAllocator.java:187)
        at org.apache.bookkeeper.common.allocator.impl.ByteBufAllocatorImpl.newDirectBuffer(ByteBufAllocatorImpl.java:164)
        at org.apache.bookkeeper.common.allocator.impl.ByteBufAllocatorImpl.newDirectBuffer(ByteBufAllocatorImpl.java:158)
        at io.netty.buffer.AbstractByteBufAllocator.directBuffer(AbstractByteBufAllocator.java:187)

eolivelli · 2019-09-08T08:39:20Z

@sijie please fix checkstyle issues

sijie · 2019-09-09T19:37:04Z

@eolivelli done

eolivelli · 2019-09-09T20:09:37Z

Let's wait for CI

eolivelli · 2019-09-10T05:32:36Z

Some tests failed with an unrelated error:
Could not use Netty Epoll event loop for bookie server:
java.lang.NoClassDefFoundError: Could not initialize class io.netty.channel.epoll.EpollEventLoop
at io.netty.channel.epoll.EpollEventLoopGroup.newChild(EpollEventLoopGroup.java:134)
at io.netty.channel.epoll.EpollEventLoopGroup.newChild(EpollEventLoopGroup.java:35)
at io.netty.util.concurrent.MultithreadEventExecutorGroup.(MultithreadEventExecutorGroup

eolivelli · 2019-09-10T05:33:33Z

rebuild java8
run integration tests
run pr validation

sijie · 2019-09-10T21:43:17Z

run pr validation

merlimat · 2019-09-10T21:45:21Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/ReadCache.java


        for (int i = 0; i < segmentsCount; i++) {
-            cacheSegments.add(Unpooled.directBuffer(segmentSize, segmentSize));
+            cacheSegments.add(Unpooled.buffer(segmentSize, segmentSize));


This is not really coming from the allocator. Rather, the intention is to really get unpooled direct memory for the read cache.

what is the side effect of being pooled?

In this case that the memory is not returned to the pool (but rather the direct mem chunks will be just GCed when the handle object is GCed)

For this case, we allocate big chunks of mem (eg: 1GB each) and we keep using them indefinitely. Effectively this is a kind of "manual pooling" of the read and write cache memory.

There would be no advantage in pooling that memory again.

but it doesn't hurt to use pooled, correct? from code maintenance perspective, isn't it clear to use allocator across the bookkeeper code base? If we need to use unpooled, we can use unpooled and document why do we need to use unpooled to make people know exact the reason behind the scene.

but it doesn't hurt to use pooled, correct?

We'd have to at least release the buffers when the ReadCache/WriteCache are closed

If we need to use unpooled, we can use unpooled and document why do we need to use unpooled to make people know exact the reason behind the scene.

Sure, that would be better to be explicitely noted why it was that way

Sure, I'm just saying that in this PR we need to add that release when switching from unpooled to pooled.

I didn't still get the point why there are benefits using unpooled buffers.

I would avoid the tracking of these buffers by the allocator. The overhead might be minimal, OTOH there's no advantage from the allocator either.

I would avoid the tracking of these buffers by the allocator. The overhead might be minimal, OTOH there's no advantage from the allocator either.

the intention is to make sure all the memory allocation is consistent across the bookie server and leverage whatever changes we made in our allocator, unless there are a very strong reason to not do so.

since there is no advantages and also no disadvantages, why not just to make things consistent and easier to maintain?

I'm not saying not to do it. just saying to add the release on close().

If you are okay with using allocator, I will fix the release part.

actually those buffers are already released on close

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/WriteCache.java

merlimat

👍

merlimat · 2019-09-11T19:27:26Z

[ERROR] /home/jenkins/jenkins-slave/workspace/bookkeeper_precommit_pullrequest_validation/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/ReadCache.java:27:8: Unused import: io.netty.buffer.Unpooled. [UnusedImports]

eolivelli · 2019-09-22T12:46:08Z

run pr validation
run integration tests
run bookkeeper-server client tests
run bookkeeper-server replication tests

eolivelli · 2019-09-24T06:49:04Z

@sijie tests are failing consistently:

2019-09-22 12:59:48,207 - ERROR - [DL-io-0:ByteBufAllocatorImpl@62] - Unable to allocate memory
io.netty.util.internal.OutOfDirectMemoryError: failed to allocate 16777216 byte(s) of direct memory (used: 2147483648, max: 2147483648)
at io.netty.util.internal.PlatformDependent.incrementMemoryCounter(PlatformDependent.java:655)
at io.netty.util.internal.PlatformDependent.allocateDirectNoCleaner(PlatformDependent.java:610)
at io.netty.buffer.PoolArena$DirectArena.allocateDirect(PoolArena.java:769)
at io.netty.buffer.PoolArena$DirectArena.newChunk(PoolArena.java:745)
at io.netty.buffer.PoolArena.allocateNormal(PoolArena.java:244)
at io.netty.buffer.PoolArena.allocate(PoolArena.java:214)
at io.netty.buffer.PoolArena.allocate(PoolArena.java:146)
at io.netty.buffer.PooledByteBufAllocator.newDirectBuffer(PooledByteBufAllocator.java:324)
at io.netty.buffer.AbstractByteBufAllocator.directBuffer(AbstractByteBufAllocator.java:185)
at org.apache.bookkeeper.common.allocator.impl.ByteBufAllocatorImpl.newDirectBuffer(ByteBufAllocatorImpl.java:164)
at org.apache.bookkeeper.common.allocator.impl.ByteBufAllocatorImpl.newDirectBuffer(ByteBufAllocatorImpl.java:158)
at io.netty.buffer.AbstractByteBufAllocator.directBuffer(AbstractByteBufAllocator.java:185)
at io.netty.buffer.AbstractByteBufAllocator.directBuffer(AbstractByteBufAllocator.java:176)
at io.netty.channel.nio.AbstractNioChannel.newDirectBuffer(AbstractNioChannel.java:448)
at io.netty.channel.nio.AbstractNioByteChannel.filterOutboundMessage(AbstractNioByteChannel.java:275)
at io.netty.channel.AbstractChannel$AbstractUnsafe.write(AbstractChannel.java:881)
at io.netty.channel.DefaultChannelPipeline$HeadContext.write(DefaultChannelPipeline.java:1391)
at io.netty.channel.AbstractChannelHandlerContext.invokeWrite0(AbstractChannelHandlerContext.java:738)
at io.netty.channel.AbstractChannelHandlerContext.invokeWrite(AbstractChannelHandlerContext.java:730)
at io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:816)
at io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:723)
at org.apache.bookkeeper.util.ByteBufList$Encoder.write(ByteBufList.java:328)
at io.netty.channel.AbstractChannelHandlerContext.invokeWrite0(AbstractChannelHandlerContext.java:738)
at io.netty.channel.AbstractChannelHandlerContext.invokeWrite(AbstractChannelHandlerContext.java:730)
at io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:816)
at io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:723)
at io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:704)
at io.netty.handler.codec.MessageToMessageEncoder.writePromiseCombiner(MessageToMessageEncoder.java:137)
at io.netty.handler.codec.MessageToMessageEncoder.write(MessageToMessageEncoder.java:119)
at io.netty.channel.AbstractChannelHandlerContext.invokeWrite0(AbstractChannelHandlerContext.java:738)
at io.netty.channel.AbstractChannelHandlerContext.invokeWrite(AbstractChannelHandlerContext.java:730)
at io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:816)
at io.netty.channel.AbstractChannelHandlerContext.write(AbstractChannelHandlerContext.java:723)
at org.apache.bookkeeper.proto.BookieProtoEncoding$RequestEncoder.write(BookieProtoEncoding.java:391)

eolivelli · 2019-10-25T15:45:49Z

@sijie do you have time to finish this work in a way that we get it into 4.10?
Tests are failing

sijie · 2019-10-26T03:51:50Z

@eolivelli feel free to move 4.11. will cherry-pick it to 4.10.1

wolfstudy · 2019-10-28T13:06:43Z

run pr validation

eolivelli

This patch needs rebase and also we have tests failing

eolivelli · 2020-04-12T16:57:00Z

@sijie what's the status of this patch?

Ghatage · 2020-06-10T22:57:24Z

run pr validation

eolivelli · 2020-06-11T08:18:02Z

@Ghatage this patch needs a rebase/resolve conflicts, it is not ready for merge.

I feel this patch is quite dangerous, it is not a good candidate for 4.11.

btw the magic word to trigger a build is
"rerun failure tests"

eolivelli · 2020-06-11T08:18:45Z

@sijie I am closing this PR. It needs a rebase and it does not run GitHub actions.
feel free to reopen once it is ready again

sijie added area/bookie type/improvement release/4.10.0 release/4.9.3 labels Sep 6, 2019

sijie added this to the 4.10.0 milestone Sep 6, 2019

sijie requested review from eolivelli, jiazhai and merlimat September 6, 2019 09:33

sijie self-assigned this Sep 6, 2019

eolivelli approved these changes Sep 6, 2019

View reviewed changes

Fix checkstyle issue

1615df3

merlimat reviewed Sep 10, 2019

View reviewed changes

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/WriteCache.java Show resolved Hide resolved

Use allocator consistently

93b16ab

merlimat approved these changes Sep 11, 2019

View reviewed changes

Fix checkstyle

f5134ed

jiazhai added release/4.11.0 and removed release/4.10.0 labels Oct 29, 2019

jiazhai modified the milestones: 4.10.0, 4.11.0 Oct 29, 2019

eolivelli requested changes Dec 22, 2019

View reviewed changes

eolivelli closed this Jun 11, 2020

wenbingshen mentioned this pull request Oct 29, 2021

Use OutOfMemoryPolicy when the direct memory is insufficient when reading the entry in ReadCache #2836

Merged

Avoid using directBuffer directly #2158

Avoid using directBuffer directly #2158

Uh oh!

Conversation

sijie commented Sep 6, 2019

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

eolivelli commented Sep 6, 2019

Uh oh!

eolivelli commented Sep 6, 2019

Uh oh!

diegosalvi commented Sep 6, 2019

Uh oh!

eolivelli commented Sep 8, 2019

Uh oh!

sijie commented Sep 9, 2019

Uh oh!

eolivelli commented Sep 9, 2019

Uh oh!

eolivelli commented Sep 10, 2019

Uh oh!

eolivelli commented Sep 10, 2019

Uh oh!

sijie commented Sep 10, 2019

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!

Uh oh!

merlimat left a comment

Choose a reason for hiding this comment

Uh oh!

merlimat commented Sep 11, 2019

Uh oh!

eolivelli commented Sep 22, 2019

Uh oh!

eolivelli commented Sep 24, 2019

Uh oh!

eolivelli commented Oct 25, 2019

Uh oh!

sijie commented Oct 26, 2019

Uh oh!

wolfstudy commented Oct 28, 2019

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

eolivelli commented Apr 12, 2020

Uh oh!

Ghatage commented Jun 10, 2020

Uh oh!

eolivelli commented Jun 11, 2020

Uh oh!

eolivelli commented Jun 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development