Implement byte array reusage in NioTransport
#27696
Conversation
Tim-Brooks
added
:Distributed Coordination/Network
|
The suggestion to pass around the Right now, I modified our generic tests cases (ESTestCase and ESIntegTestCase) to only check that the page cache recycler has released all pages |
s1monw
left a comment
s1monw
left a comment
There was a problem hiding this comment.
I left some minors. looks good
| public void close() { | ||
| Page page; | ||
| while ((page = pages.pollFirst()) != null) { | ||
| page.close(); |
There was a problem hiding this comment.
should we do this similar to IOUtils.close where we catch the excetpion and rethrow afterwards?
There was a problem hiding this comment.
now thta we close should we prevent access after we are closed?
| releasable.close(); | ||
| } | ||
|
|
||
| private ByteBuffer byteBuffer() { |
There was a problem hiding this comment.
I think we can just access the member directly.
| private int offset = 0; | ||
|
|
||
| public InboundChannelBuffer() { | ||
| this(() -> ByteBuffer.wrap(new byte[PAGE_SIZE])); |
There was a problem hiding this comment.
can we remove this default ctor and pass the non-recycling closure in the test directly. I think it's just one place.
|
@s1monw I've made changes based on your review. |
|
I like the approach! |
| while ((page = pages.pollFirst()) != null) { | ||
| try { | ||
| page.close(); | ||
| } catch (RuntimeException e) { |
There was a problem hiding this comment.
you can use ExceptionsHelper#rethrowAndSuppress here
This is related to #27563. This commit modifies the InboundChannelBuffer to support releasable byte pages. These byte pages are provided by the PageCacheRecycler. The PageCacheRecycler must be passed to the Transport with this change.
* master: (414 commits) Set ACK timeout on indices service test Implement byte array reusage in `NioTransport` (elastic#27696) [TEST] Remove leftover ES temp directories before Vagrant tests (elastic#27722) Cleanup split strings by comma method Remove unused import from AliasResolveRoutingIT Add read timeouts to http module (elastic#27713) Fix routing with leading or trailing whitespace remove await fix from FullClusterRestartIT.testRecovery Add missing 's' to tmpdir name (elastic#27721) [Issue-27716]: CONTRIBUTING.md IntelliJ configurations settings are confusing. (elastic#27717) [TEST] Now actually wait for merges Test out of order delivery of append only index and retry with an intermediate delete [TEST] remove code duplications in RequestTests [Tests] Add test for GeoShapeFieldType#setStrategyName (elastic#27703) Remove unused *Commit* classes (elastic#27714) Add test for writer operation buffer accounting (elastic#27707) [TEST] Wait for merging to complete before testing breaker Add Open Index API to the high level REST client (elastic#27574) Correcting some minor typos in comments Add unreleased v5.6.6 version ...
* master: Fix index with unknown setting test Remove internal channel tracking in transports (elastic#27711) Improve error msg when a field name contains only white spaces (elastic#27709) Do not open indices with broken settings Set ACK timeout on indices service test Implement byte array reusage in `NioTransport` (elastic#27696) [TEST] Remove leftover ES temp directories before Vagrant tests (elastic#27722) Cleanup split strings by comma method Remove unused import from AliasResolveRoutingIT Add read timeouts to http module (elastic#27713) Fix routing with leading or trailing whitespace remove await fix from FullClusterRestartIT.testRecovery Add missing 's' to tmpdir name (elastic#27721) [Issue-27716]: CONTRIBUTING.md IntelliJ configurations settings are confusing. (elastic#27717) [TEST] Now actually wait for merges
4 participants
This is related to #27563. This commit modifies the
InboundChannelBufferto support releasable byte pages. These bytepages are provided by the
PageCacheRecycler. ThePageCacheRecyclermust be passed to the
Transportwith this change.