[GitHub] merlimat commented on a change in pull request #1755: Configure Netty allocator in bookie and client
merlimat commented on a change in pull request #1755: Configure Netty allocator in bookie and client URL: https://github.com/apache/bookkeeper/pull/1755#discussion_r231783094 ## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java ## @@ -195,6 +198,12 @@ // Stats protected static final String ENABLE_TASK_EXECUTION_STATS = "enableTaskExecutionStats"; +// Allocator configuration +protected static final String ALLOCATOR_POOLING_POLICY = "allocatorPoolingPolicy"; Review comment: @sijie I missed it since it was hidden by github. Fixed now This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] merlimat commented on a change in pull request #1755: Configure Netty allocator in bookie and client
merlimat commented on a change in pull request #1755: Configure Netty
allocator in bookie and client
URL: https://github.com/apache/bookkeeper/pull/1755#discussion_r231782998
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java
##
@@ -124,6 +124,7 @@ public BookieServer(ServerConfiguration conf, StatsLogger
statsLogger)
*/
public void setExceptionHandler(UncaughtExceptionHandler exceptionHandler)
{
this.uncaughtExceptionHandler = exceptionHandler;
+this.bookie.setExceptionHandler(exceptionHandler);
Review comment:
Fixed
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat commented on a change in pull request #1755: Configure Netty allocator in bookie and client
merlimat commented on a change in pull request #1755: Configure Netty
allocator in bookie and client
URL: https://github.com/apache/bookkeeper/pull/1755#discussion_r228687243
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java
##
@@ -416,14 +419,7 @@ protected ChannelFuture connect() {
bootstrap.channel(NioSocketChannel.class);
}
-ByteBufAllocator allocator;
-if (this.conf.isNettyUsePooledBuffers()) {
Review comment:
Sure
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat commented on a change in pull request #1755: Configure Netty allocator in bookie and client
merlimat commented on a change in pull request #1755: Configure Netty
allocator in bookie and client
URL: https://github.com/apache/bookkeeper/pull/1755#discussion_r228687202
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java
##
@@ -369,7 +391,30 @@ public BookKeeper(ClientConfiguration conf, ZooKeeper zk)
*/
public BookKeeper(ClientConfiguration conf, ZooKeeper zk, EventLoopGroup
eventLoopGroup)
throws IOException, InterruptedException, BKException {
-this(conf, validateZooKeeper(zk),
validateEventLoopGroup(eventLoopGroup), NullStatsLogger.INSTANCE,
+this(conf, validateZooKeeper(zk),
validateEventLoopGroup(eventLoopGroup), null);
+}
+
+/**
+ * Create a bookkeeper client but use the passed in zookeeper client and
+ * client event loop group instead of instantiating those.
+ *
+ * @param conf
+ * Client Configuration Object
+ * {@link ClientConfiguration}
+ * @param zk
+ * Zookeeper client instance connected to the zookeeper with which
+ * the bookies have registered. The ZooKeeper client must be
connected
+ * before it is passed to BookKeeper. Otherwise a KeeperException
is thrown.
+ * @param eventLoopGroup
+ * An event loop group that will be used to create connections to
the bookies
+ * @throws IOException
+ * @throws InterruptedException
+ * @throws BKException in the event of a bookkeeper connection error
+ */
+public BookKeeper(ClientConfiguration conf, ZooKeeper zk, EventLoopGroup
eventLoopGroup,
Review comment:
Ok, I was doing this since this is the constructor we use in Pulsar :)
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat commented on a change in pull request #1755: Configure Netty allocator in bookie and client
merlimat commented on a change in pull request #1755: Configure Netty
allocator in bookie and client
URL: https://github.com/apache/bookkeeper/pull/1755#discussion_r228687050
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java
##
@@ -634,6 +639,21 @@ public Bookie(ServerConfiguration conf, StatsLogger
statsLogger)
this.indexDirsManager = createIndexDirsManager(conf, diskChecker,
statsLogger.scope(LD_INDEX_SCOPE),
this.ledgerDirsManager);
+this.allocator = ByteBufAllocatorBuilder.create()
+.poolingPolicy(conf.getAllocatorPoolingPolicy())
+.poolingConcurrency(conf.getAllocatorPoolingConcurrency())
+.outOfMemoryPolicy(conf.getAllocatorOutOfMemoryPolicy())
+.outOfMemoryListener((ex) -> {
+try {
+LOG.error("Unable to allocate memory, exiting bookie",
ex);
+shutdown(-1);
+} finally {
+Runtime.getRuntime().halt(-1);
Review comment:
How should be pass it?
What I wanted to achieve here is:
* Try to shutdown bookie gracefully (mainly to close ZK session such that
new ledgers won't choose this bookie)
* No matter what, bring the process down after that attempt. This would
protect from a thread hanging on in background to keep JVM alive.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat commented on a change in pull request #1755: Configure Netty allocator in bookie and client
merlimat commented on a change in pull request #1755: Configure Netty
allocator in bookie and client
URL: https://github.com/apache/bookkeeper/pull/1755#discussion_r228686668
##
File path: bookkeeper-server/pom.xml
##
@@ -37,6 +37,18 @@
+
+ org.apache.bookkeeper
+ bookkeeper-common-allocator
+ ${project.parent.version}
+
+
Review comment:
Good point, will fix that. In general we should also refactor netty
dependency to not use netty-all, but that's a completely different story
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] merlimat commented on a change in pull request #1755: Configure Netty allocator in bookie and client
merlimat commented on a change in pull request #1755: Configure Netty allocator in bookie and client URL: https://github.com/apache/bookkeeper/pull/1755#discussion_r228659329 ## File path: conf/bk_server.conf ## @@ -956,3 +956,53 @@ storage.serve.readonly.tables=false # the cluster controller schedule interval, in milliseconds. default is 30 seconds. storage.cluster.controller.schedule.interval.ms=3 + + +# +## Netty Allocator Settings +# + +# Define the memory pooling policy. +# Available options are: +# - PooledDirect: Use Direct memory for all buffers and pool the memory. +# Direct memory will avoid the overhead of JVM GC and most +# memory copies when reading and writing to socket channel. +# Pooling will add memory space overhead due to the fact that +# there will be fragmentation in the allocator and that threads +# will keep a portion of memory as thread-local to avoid +# contention when possible. +# - UnpooledHeap: Allocate memory from JVM heap without any pooling. +# This option has the least overhead in terms of memory usage +# since the memory will be automatically reclaimed by the +# JVM GC but might impose a performance penalty at high +# throughput. +# Default is: PooledDirect +# allocatorPoolingPolicy=PooledDirect + +# Controls the amount of concurrency for the memory pool. +# Default is to have a number of allocator arenas equals to 2 * CPUS. Review comment: That's current Netty default This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
