Copilot commented on code in PR #3625:
URL: https://github.com/apache/celeborn/pull/3625#discussion_r2924045046
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1949,16 +1949,22 @@ object CelebornConf extends Logging {
.booleanConf
.createWithDefault(false)
- val NETWORK_MEMORY_ALLOCATOR_POOLED: ConfigEntry[Boolean] =
- buildConf("celeborn.network.memory.allocator.pooled")
+ val NETWORK_MEMORY_ALLOCATOR_TYPE: ConfigEntry[String] =
+ buildConf("celeborn.network.memory.allocator.type")
.categories("network")
.internal
- .version("0.6.0")
- .doc("If disabled, always use UnpooledByteBufAllocator for aggressive
memory reclamation, " +
- "this is helpful for cases that worker has high memory usage even
after triming. " +
- "Disabling would cause performace degression and higher CPU usage.")
- .booleanConf
- .createWithDefault(true)
+ .version("0.7.0")
+ .doc("Specifies netty memory allocator type including: " +
+ s"${NettyMemoryAllocatorType.POOLED.name}: use PooledByteBufAllocator,
which is the default and recommended for better performance. " +
+ s"${NettyMemoryAllocatorType.UNPOOLED.name}: use
UnpooledByteBufAllocator, which is more aggressive in memory reclamation and
may cause performance depression and higher CPU usage. " +
+ s"${NettyMemoryAllocatorType.ADAPTIVE.name}: use
AdaptiveByteBufAllocator, which is recommended to roll out usage slowly, and to
carefully monitor application performance in the process.")
+ .stringConf
+ .transform(_.toUpperCase(Locale.ROOT))
+ .checkValues(Set(
+ NettyMemoryAllocatorType.POOLED.name,
+ NettyMemoryAllocatorType.UNPOOLED.name,
+ NettyMemoryAllocatorType.ADAPTIVE.name))
+ .createWithDefault(NettyMemoryAllocatorType.POOLED.name)
Review Comment:
New config parsing paths (value normalization + validation + old-key
alternative translation, if added) are not covered by tests. Since
`common/src/test/scala/org/apache/celeborn/common/CelebornConfSuite.scala`
already covers config alternatives and validation, please add a small unit test
for `celeborn.network.memory.allocator.type` (and the deprecated
`...allocator.pooled` fallback) to prevent regressions.
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1949,16 +1949,22 @@ object CelebornConf extends Logging {
.booleanConf
.createWithDefault(false)
- val NETWORK_MEMORY_ALLOCATOR_POOLED: ConfigEntry[Boolean] =
- buildConf("celeborn.network.memory.allocator.pooled")
+ val NETWORK_MEMORY_ALLOCATOR_TYPE: ConfigEntry[String] =
+ buildConf("celeborn.network.memory.allocator.type")
.categories("network")
.internal
Review Comment:
`celeborn.network.memory.allocator.type` is marked as `.internal`, which
makes it non-public and likely excludes it from generated configuration docs.
Since this PR introduces it as a user-facing setting (and migration guide
references it), it should be public (remove `.internal`) or the docs generation
should be updated so users can discover it.
```suggestion
```
##########
common/src/main/java/org/apache/celeborn/common/network/util/NettyUtils.java:
##########
@@ -169,10 +179,10 @@ public static synchronized ByteBufAllocator
getSharedByteBufAllocator(
CelebornConf conf, AbstractSource source, boolean allowCache) {
final int index = allowCache ? 0 : 1;
if (_sharedByteBufAllocator[index] == null) {
+ NettyMemoryAllocatorType allocatorType =
conf.networkMemoryAllocatorType();
_sharedByteBufAllocator[index] =
- createByteBufAllocator(
- conf.networkMemoryAllocatorPooled(), true, allowCache,
conf.networkAllocatorArenas());
- if (conf.networkMemoryAllocatorPooled()) {
+ createByteBufAllocator(allocatorType, true, allowCache,
conf.networkAllocatorArenas());
+ if (allocatorType == NettyMemoryAllocatorType.POOLED) {
pooledByteBufAllocators.add((PooledByteBufAllocator)
_sharedByteBufAllocator[index]);
Review Comment:
Selecting `ADAPTIVE` will prevent pinned-memory accounting:
`pooledByteBufAllocators` only tracks `PooledByteBufAllocator`, so when
`allocatorType` is `ADAPTIVE` MemoryManager's `getNettyPinnedDirectMemory()`
will effectively return 0 (see `MemoryManager#getNettyPinnedDirectMemory`).
This can break pinned-memory based pause/resume behavior. Consider either (a)
tracking/adapting pinned-memory metrics for Adaptive allocators if Netty
exposes them, or (b) explicitly disabling pinned-memory checks when allocator
type is not `POOLED`.
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1949,16 +1949,22 @@ object CelebornConf extends Logging {
.booleanConf
.createWithDefault(false)
- val NETWORK_MEMORY_ALLOCATOR_POOLED: ConfigEntry[Boolean] =
- buildConf("celeborn.network.memory.allocator.pooled")
+ val NETWORK_MEMORY_ALLOCATOR_TYPE: ConfigEntry[String] =
+ buildConf("celeborn.network.memory.allocator.type")
.categories("network")
.internal
- .version("0.6.0")
- .doc("If disabled, always use UnpooledByteBufAllocator for aggressive
memory reclamation, " +
- "this is helpful for cases that worker has high memory usage even
after triming. " +
- "Disabling would cause performace degression and higher CPU usage.")
- .booleanConf
- .createWithDefault(true)
+ .version("0.7.0")
+ .doc("Specifies netty memory allocator type including: " +
+ s"${NettyMemoryAllocatorType.POOLED.name}: use PooledByteBufAllocator,
which is the default and recommended for better performance. " +
+ s"${NettyMemoryAllocatorType.UNPOOLED.name}: use
UnpooledByteBufAllocator, which is more aggressive in memory reclamation and
may cause performance depression and higher CPU usage. " +
Review Comment:
Docstring typo: "performance depression" reads like a mood/metric; the
intended meaning seems to be "performance degradation" (or similar). Please
adjust wording to avoid confusion in user-facing config docs/help text.
```suggestion
s"${NettyMemoryAllocatorType.UNPOOLED.name}: use
UnpooledByteBufAllocator, which is more aggressive in memory reclamation and
may cause performance degradation and higher CPU usage. " +
```
##########
docs/migration.md:
##########
@@ -33,6 +33,8 @@ license: |
- Since 0.7.0, Celeborn changed the default value of
`celeborn.worker.directMemoryRatioForReadBuffer` from `0.1` to `0.35`, which
means read buffer threshold of buffer dispatcher is max direct memory * 0.35 at
default.
+- Since 0.7.0, Celeborn deprecate `celeborn.network.memory.allocator.pooled`.
Please use `celeborn.network.memory.allocator.type` instead.
+
# Upgrading from 0.5 to 0.6
- Since 0.6.0, Celeborn deprecate
`celeborn.client.spark.fetch.throwsFetchFailure`. Please use
`celeborn.client.spark.stageRerun.enabled` instead.
Review Comment:
The migration entry says Celeborn "deprecate" (grammar) and also implies the
old key still exists. Please change to "deprecates" and ensure the statement
matches actual behavior (either keep the old key working with a deprecation
path, or update the migration note to say it was removed/ignored in 0.7.0).
```suggestion
- Since 0.7.0, Celeborn deprecates and removes
`celeborn.network.memory.allocator.pooled` (the key is ignored from 0.7.0).
Please use `celeborn.network.memory.allocator.type` instead.
# Upgrading from 0.5 to 0.6
- Since 0.6.0, Celeborn deprecates
`celeborn.client.spark.fetch.throwsFetchFailure`. Please use
`celeborn.client.spark.stageRerun.enabled` instead.
```
##########
common/src/main/java/org/apache/celeborn/common/network/util/NettyUtils.java:
##########
@@ -206,13 +216,10 @@ public static ByteBufAllocator getByteBufAllocator(
} else {
arenas = conf.getCelebornConf().networkAllocatorArenas();
}
+ NettyMemoryAllocatorType allocatorType =
conf.getCelebornConf().networkMemoryAllocatorType();
ByteBufAllocator allocator =
- createByteBufAllocator(
- conf.getCelebornConf().networkMemoryAllocatorPooled(),
- conf.preferDirectBufs(),
- allowCache,
- arenas);
- if (conf.getCelebornConf().networkMemoryAllocatorPooled()) {
+ createByteBufAllocator(allocatorType, conf.preferDirectBufs(),
allowCache, arenas);
+ if (allocatorType == NettyMemoryAllocatorType.POOLED) {
pooledByteBufAllocators.add((PooledByteBufAllocator) allocator);
Review Comment:
Same pinned-memory accounting issue for per-module allocators: only `POOLED`
allocators are recorded in `pooledByteBufAllocators`, so `ADAPTIVE` allocators
will not be reflected in pinned direct memory tracking used by the worker
memory manager. Please ensure the pinned-memory based logic is correct (either
add support for Adaptive allocator metrics or guard pinned-memory features when
allocator type != `POOLED`).
##########
common/src/main/java/org/apache/celeborn/common/network/util/NettyMemoryAllocatorType.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.celeborn.common.network.util;
+
+import io.netty.buffer.AdaptiveByteBufAllocator;
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.buffer.PooledByteBufAllocator;
+import io.netty.buffer.UnpooledByteBufAllocator;
+
+/** Netty memory allocator type of {@link ByteBufAllocator}. */
+public enum NettyMemoryAllocatorType {
+ /** A pooled {@link ByteBufAllocator}: {@link PooledByteBufAllocator}. */
+ POOLED,
+ /** A un-pooled {@link ByteBufAllocator}: {@link UnpooledByteBufAllocator}.
*/
+ UNPOOLED,
+ /** An auto-tuning pooling {@link ByteBufAllocator}: {@link
AdaptiveByteBufAllocator}. */
Review Comment:
This new enum has several unused imports (`AdaptiveByteBufAllocator`,
`ByteBufAllocator`, `PooledByteBufAllocator`, `UnpooledByteBufAllocator`) that
are only referenced from Javadoc. Consider removing these imports and using
fully-qualified names in the Javadoc `{@link ...}` tags (or otherwise
referencing the types in code) to avoid unused-import warnings / build failures
under stricter compiler settings.
```suggestion
/** Netty memory allocator type of {@link io.netty.buffer.ByteBufAllocator}.
*/
public enum NettyMemoryAllocatorType {
/** A pooled {@link io.netty.buffer.ByteBufAllocator}: {@link
io.netty.buffer.PooledByteBufAllocator}. */
POOLED,
/** A un-pooled {@link io.netty.buffer.ByteBufAllocator}: {@link
io.netty.buffer.UnpooledByteBufAllocator}. */
UNPOOLED,
/** An auto-tuning pooling {@link io.netty.buffer.ByteBufAllocator}:
{@link io.netty.buffer.AdaptiveByteBufAllocator}. */
```
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -1949,16 +1949,22 @@ object CelebornConf extends Logging {
.booleanConf
.createWithDefault(false)
- val NETWORK_MEMORY_ALLOCATOR_POOLED: ConfigEntry[Boolean] =
- buildConf("celeborn.network.memory.allocator.pooled")
+ val NETWORK_MEMORY_ALLOCATOR_TYPE: ConfigEntry[String] =
+ buildConf("celeborn.network.memory.allocator.type")
.categories("network")
Review Comment:
The migration guide says `celeborn.network.memory.allocator.pooled` is
deprecated, but this PR removes the old config entry entirely. To keep upgrades
painless, consider adding
`.withAlternative("celeborn.network.memory.allocator.pooled", v => if
(v.trim.toBoolean) "POOLED" else "UNPOOLED")` on the new
`celeborn.network.memory.allocator.type` entry (so the old key still works and
triggers a deprecation warning).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
