[CELEBORN-1815] Support UnpooledByteBufAllocator #3043
Conversation
| source.addGauge( | ||
| MetricRegistry.name(metricPrefix, "usedDirectMemory"), | ||
| labels, | ||
| unpooledMetric::usedDirectMemory); |
There was a problem hiding this comment.
UnpooledByteBufAllocator only supports these two metrics
| @@ -45,7 +45,7 @@ public FlinkTransportClientFactory( | |||
| TransportContext context, List<TransportClientBootstrap> bootstraps, int bufferSizeBytes) { | |||
| super(context, bootstraps); | |||
| bufferSuppliers = JavaUtils.newConcurrentHashMap(); | |||
| this.pooledAllocator = new UnpooledByteBufAllocator(true); | |||
| this.allocator = new UnpooledByteBufAllocator(true); | |||
There was a problem hiding this comment.
Interesting... Flink client overrides it to forcibly use UnpooledByteBufAllocator
There was a problem hiding this comment.
This is related to this PR. The Flink client's memory is quite limited.
#1324
There was a problem hiding this comment.
@FMX Thanks for the information. BTW, it's better to leave some brief comments to explain the special logic
| boolean allowDirectBufs, boolean allowCache, int numCores) { | ||
| if (numCores == 0) { | ||
| numCores = Runtime.getRuntime().availableProcessors(); | ||
| private static ByteBufAllocator createByteBufAllocator( |
There was a problem hiding this comment.
The method comments should also be updated
There was a problem hiding this comment.
update: move the existing comment to the caller side, add new comments to explain each parameters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3043 +/- ##
==========================================
- Coverage 32.68% 32.67% -0.00%
==========================================
Files 336 336
Lines 20032 20053 +21
Branches 1792 1796 +4
==========================================
+ Hits 6546 6551 +5
- Misses 13123 13137 +14
- Partials 363 365 +2 ☔ View full report in Codecov by Sentry. |
|
Thanks, merged to main(0.6) |
What changes were proposed in this pull request?
This PR introduces a configuration
celeborn.network.memory.allocator.pooledto allow users to disablePooledByteBufAllocatorglobally and always useUnpooledByteBufAllocator.Why are the changes needed?
In some extreme cases, the Netty's
PooledByteBufAllocatormight have tons of 4MiB chunks but only a few sizes of the capacity are used by the real data(see #3018), for scenarios that stability is important than performance, it's desirable to allow users to disable thePooledByteBufAllocatorglobally.Does this PR introduce any user-facing change?
Add a new feature, disabled by default.
How was this patch tested?
Pass UT to ensure correctness. Performance and memory impact need to be verified in the production scale cluster.