Netty allocator wrapper #1754
Netty allocator wrapper #1754
Conversation
merlimat
requested review from
dlg99,
eolivelli,
ivankelly,
jiazhai,
jvrao,
reddycharan and
sijie
merlimat
force-pushed
the
allocator
branch
2 times, most recently
from
fe6d5dc to
adb8bcf
Compare
ivankelly
left a comment
ivankelly
left a comment
There was a problem hiding this comment.
lgtm. Please update the description with the relevant section. Specifically, the 'why'
| * | ||
| * <p>Default is to use a new instance of {@link PooledByteBufAllocator}. | ||
| */ | ||
| ByteBufAllocatorBuilder pooledAllocator(ByteBufAllocator ooledAllocator); |
| @@ -0,0 +1,60 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
any reason why we add another module, not add this to bookkeeper-common? I think allocator is a common enough component in current bookkeeper codebase, since bookkeeper heavily dependend on netty's buffers.
There was a problem hiding this comment.
It's to include bookkeeper-common-allocator in pulsar-client that currently doesn't depend on bookkeeper-common, because it would pull in another set of dependencies.
bookkeeper-common-allocator/pom.xml
Outdated
| <dependencies> | ||
| <dependency> | ||
| <groupId>io.netty</groupId> | ||
| <artifactId>netty-all</artifactId> |
There was a problem hiding this comment.
these classes only dependend on netty-buffer. netty-all includes all classes which are not needed for a "common" module.
There was a problem hiding this comment.
Only reason is that throughout the code we use netty-all everywhere. If we use netty-buffer here, then we need to exclude it later or we'd be pulling both dependencies.
In any case, I agree that netty-all is a bit overkill, not just here but throughout the modules. We should take a look at breaking all netty dependencies down.
There was a problem hiding this comment.
Fixed to require netty-buffer
|
run integration tests |
|
run integration tests |
|
This allocator is very useful to address some of the issues I have encountered using netty. |
|
IGNORE IT CI |
### Motivation This is based on #1754. Adding the code to configure and use the allocator wrapper in bookie and client. (I'll rebase once the first PR is merged) Reviewers: Ivan Kelly <ivank@apache.org>, Enrico Olivelli <eolivelli@gmail.com>, Sijie Guo <sijie@apache.org> This closes #1755 from merlimat/use-allocator
5 participants
Motivation
Configuring the correct JVM memory settings and cache sizes for a BookKeeper cluster should be simplified.
There are currently many knobs in Netty or JVM flags for different components and while with a good setup the systems is very stable, it's easy to setup non-optimal configurations which might result in OutOfMemory errors under load.
Ideally, there should be very minimal configuration required to bring up a BookKeeper cluster that can work under a wide set of traffic loads. In any case, we should prefer to automatically fallback to slower alternatives, when possible, instead of throwing OOM exceptions.
Changes
bookkeeper-common-allocatormodule to have a no-dependencues module that can be used directly from Pulsar client library too (which doesn't depend on BK).