Remove option to enable direct buffer pooling #47956
Conversation
This commit removes the option to change the netty system properties to reenable the direct buffer pooling. It also removes the need for us to disable the buffer pooling in the system properties file. Instead, we programmatically craete an allocator that is used by our networking layer. This commit does introduce an Elasticsearch property which allows the user to fallback on the netty default allocator. If they choose this option, they can configure the default allocator how they wish using the standard netty properties.
Tim-Brooks
added
>non-issue
:Distributed Coordination/Network
|
Pinging @elastic/es-distributed (:Distributed/Network) |
|
When we disabled direct buffer pooling in 7.4, we agreed that once it was determined to be stable we could remove the fallback mode. This commit moves to a world where we programmatically assign the buffer allocator and do not use the default netty one. This commit does add a system property to allow expert users to fallback on the netty allocator (and therefore configure it how they like). We still need to discuss this. The name of the system property is tentative. And this has some friction with the "unpooled" system property which we currently apply. We should perhaps move to a world where JvmErgonomics just tells us if we should use the unpooled allocator (based on heap size). However, then this has some tension with testing (we could add an Elasticsearch system property for the unpooled allocator that the tests apply |
I wonder if we should even allow this? I'm having a hard time seeing any situation where this would be helpful. On the other hand, if we keep this option around we have to continue to support/test it. |
distribution/tools/launchers/src/main/java/org/elasticsearch/tools/launchers/JvmErgonomics.java
Outdated
Show resolved
Hide resolved
modules/transport-netty4/src/main/java/org/elasticsearch/transport/netty4/Netty4Transport.java
Outdated
Show resolved
Hide resolved
distribution/tools/launchers/src/main/java/org/elasticsearch/tools/launchers/JvmErgonomics.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public ByteBuf directBuffer(int initialCapacity, int maxCapacity) { | ||
| // TODO: Currently the Netty SslHandler requests direct ByteBufs even when interacting with the |
There was a problem hiding this comment.
NIT: Maybe link the relevant Netty issue in the TODO?
modules/transport-netty4/src/main/java/org/elasticsearch/transport/NettyAllocator.java
Outdated
Show resolved
Hide resolved
modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpClient.java
Show resolved
Hide resolved
modules/transport-netty4/src/main/java/org/elasticsearch/transport/NettyAllocator.java
Show resolved
Hide resolved
| @@ -202,7 +202,7 @@ public CompositeByteBuf compositeDirectBuffer(int maxNumComponents) { | |||
|
|
|||
| @Override | |||
| public boolean isDirectBufferPooled() { | |||
| return delegate.isDirectBufferPooled(); | |||
| return false; | |||
There was a problem hiding this comment.
perhaps assert delegate.isDirectBufferPooled() == false?
This commit removes the option to change the netty system properties to reenable the direct buffer pooling. It also removes the need for us to disable the buffer pooling in the system properties file. Instead, we programmatically craete an allocator that is used by our networking layer. This commit does introduce an Elasticsearch property which allows the user to fallback on the netty default allocator. If they choose this option, they can configure the default allocator how they wish using the standard netty properties.
* elastic/master: [Docs] Fix opType options in IndexRequest API example. (elastic#48290) Simplify Shard Snapshot Upload Code (elastic#48155) Mute ClassificationIT tests (elastic#48338) Reenable azure repository tests and remove some randomization in http servers (elastic#48283) Use an env var for the classpath of jar hell task (elastic#48240) Refactor FIPS BootstrapChecks to simple checks (elastic#47499) Add "format" to "range" queries resulted from optimizing a logical AND (elastic#48073) [DOCS][Transform] document limitation regarding rolling upgrade with 7.2, 7.3 (elastic#48118) Fail with a better error when if there are no ingest nodes (elastic#48272) Fix executing enrich policies stats (elastic#48132) Use MultiFileTransfer in CCR remote recovery (elastic#44514) Make BytesReference an interface (elastic#48171) Also validate source index at put enrich policy time. (elastic#48254) Add 'javadoc' task to lifecycle check tasks (elastic#48214) Remove option to enable direct buffer pooling (elastic#47956) [DOCS] Add 'Selecting gateway and seed nodes' section to CCS docs (elastic#48297) Add Enrich Origin (elastic#48098) fix incorrect comparison (elastic#48208)
With this commit we align Rally with Elasticsearch's defaults as implemented in elastic/elasticsearch#47956. Relates elastic/elasticsearch#47956
With this commit we align Rally with Elasticsearch's defaults as implemented in elastic/elasticsearch#47956. Relates elastic/elasticsearch#47956 Relates #33
With this commit we align Rally with Elasticsearch's defaults as implemented in elastic/elasticsearch#47956. Relates elastic/elasticsearch#47956 Relates #33
This commit removes the option to change the netty system properties to
reenable the direct buffer pooling. It also removes the need for us to
disable the buffer pooling in the system properties file. Instead, we
programmatically craete an allocator that is used by our networking
layer.
This commit does introduce an Elasticsearch property which allows the
user to fallback on the netty default allocator. If they choose this
option, they can configure the default allocator how they wish using the
standard netty properties.