[SPARK-41376][CORE] Correct the Netty preferDirectBufs check logic on executor start #38901

pan3793 · 2022-12-04T13:24:07Z

What changes were proposed in this pull request?

Fix the condition for judging Netty prefer direct memory on executor start, the logic should match org.apache.spark.network.client.TransportClientFactory.

Why are the changes needed?

The check logical was added in SPARK-27991, the original intention is to avoid potential Netty OOM issue when Netty uses direct memory to consume shuffle data, but the condition is not sufficient, this PR updates the logic to match org.apache.spark.network.client.TransportClientFactory

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Manual testing.

… spark.shuffle.io.preferDirectBufs

pan3793 · 2022-12-04T13:26:19Z

cc @Ngone51 @mridulm

core/src/main/scala/org/apache/spark/executor/CoarseGrainedExecutorBackend.scala

AmplabJenkins · 2022-12-04T22:26:17Z

Can one of the admins verify this patch?

pan3793 · 2022-12-05T02:31:26Z

@srowen code and comment have been updated.

srowen · 2022-12-05T02:33:16Z

It's a bigger change, maybe someone with more understanding of the code can evaluate this

mridulm · 2022-12-05T04:57:05Z

+CC @Ngone51 (who last updated this) and @cloud-fan (who merged the commit).

core/src/main/scala/org/apache/spark/executor/CoarseGrainedExecutorBackend.scala

Ngone51 · 2022-12-06T02:27:37Z

core/src/main/scala/org/apache/spark/executor/CoarseGrainedExecutorBackend.scala

+      val preferDirectBufsForSharedByteBufAllocators =
+        env.conf.getBoolean("spark.network.io.preferDirectBufs", true)
+      val preferDirectBufs =
+        env.conf.getBoolean("spark.shuffle.io.preferDirectBufs", true)


Did you see where is this conf accessed? I didn't find it used anywhere.

it's a combination of ("spark", module, "io.preferDirectBufs"), for shuffle client, the module is "shuffle", see org.apache.spark.network.util.TransportConf

Ngone51 · 2022-12-06T02:28:38Z

core/src/main/scala/org/apache/spark/executor/CoarseGrainedExecutorBackend.scala

@@ -85,7 +85,19 @@ private[spark] class CoarseGrainedExecutorBackend(

    logInfo("Connecting to driver: " + driverUrl)
    try {
-      if (PlatformDependent.directBufferPreferred() &&
+      // The following logic originally comes from the constructor of


Shall we add this logic into a NettyUitls function?

I'm afraid not, NettyUtils located at network-common, it can not access SparkConf which in core

How about NettyUtils just accept boolean inputs, not config

Updated, @Ngone51 would you please take a look again?

LuciferYang

+1, LGTM

srowen

Seems OK; can we or should we share this logic elsewhere? can it be called at other sites or is this the only place this check happens?

pan3793 · 2022-12-07T16:14:11Z

is this the only place this check happens?

I think yes.

The following logic looks similar but not same, I think the existing code is clear enough.

spark/common/network-common/src/main/java/org/apache/spark/network/server/TransportServer.java

Lines 73 to 79 in 1d2159f

    
           if (conf.sharedByteBufAllocators()) { 
        
             this.pooledAllocator = NettyUtils.getSharedPooledByteBufAllocator( 
        
                 conf.preferDirectBufsForSharedByteBufAllocators(), true /* allowCache */); 
        
           } else { 
        
             this.pooledAllocator = NettyUtils.createPooledByteBufAllocator( 
        
                 conf.preferDirectBufs(), true /* allowCache */, conf.serverThreads()); 
        
           }

spark/common/network-common/src/main/java/org/apache/spark/network/client/TransportClientFactory.java

Lines 110 to 116 in 1d2159f

    
           if (conf.sharedByteBufAllocators()) { 
        
             this.pooledAllocator = NettyUtils.getSharedPooledByteBufAllocator( 
        
                 conf.preferDirectBufsForSharedByteBufAllocators(), false /* allowCache */); 
        
           } else { 
        
             this.pooledAllocator = NettyUtils.createPooledByteBufAllocator( 
        
                 conf.preferDirectBufs(), false /* allowCache */, conf.clientThreads()); 
        
           }

srowen · 2022-12-08T00:15:26Z

Merged to master

dongjoon-hyun · 2022-12-08T03:43:23Z

Thank you all.
If this is caused by SPARK-27991 at Spark 3.2.0, do we need to backport to branch-3.3 and 3.2?

pan3793 · 2022-12-08T03:45:45Z

Thanks @dongjoon-hyun for checking, I think you are right, it should be ported to 3.2/3.3

dongjoon-hyun · 2022-12-08T07:05:23Z

Could you make a backporting PR, @pan3793 ?

… executor start ### What changes were proposed in this pull request? Fix the condition for judging Netty prefer direct memory on executor start, the logic should match `org.apache.spark.network.client.TransportClientFactory`. ### Why are the changes needed? The check logical was added in SPARK-27991, the original intention is to avoid potential Netty OOM issue when Netty uses direct memory to consume shuffle data, but the condition is not sufficient, this PR updates the logic to match `org.apache.spark.network.client.TransportClientFactory` ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Manual testing. Closes apache#38901 from pan3793/SPARK-41376. Authored-by: Cheng Pan <chengpan@apache.org> Signed-off-by: Sean Owen <srowen@gmail.com>

pan3793 · 2022-12-08T11:26:32Z

@dongjoon-hyun I opened #38981 (for branch-3.3) and #38982 (for branch-3.2), please take a look.

…ic on executor start ### What changes were proposed in this pull request? Backport #38901 to branch-3.3. Fix the condition for judging Netty prefer direct memory on executor start, the logic should match `org.apache.spark.network.client.TransportClientFactory`. ### Why are the changes needed? The check logical was added in SPARK-27991, the original intention is to avoid potential Netty OOM issue when Netty uses direct memory to consume shuffle data, but the condition is not sufficient, this PR updates the logic to match `org.apache.spark.network.client.TransportClientFactory` ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Manual testing. Closes #38981 from pan3793/SPARK-41376-3.3. Authored-by: Cheng Pan <chengpan@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

…ic on executor start ### What changes were proposed in this pull request? Backport #38901 to branch-3.2. Fix the condition for judging Netty prefer direct memory on executor start, the logic should match `org.apache.spark.network.client.TransportClientFactory`. ### Why are the changes needed? The check logical was added in SPARK-27991, the original intention is to avoid potential Netty OOM issue when Netty uses direct memory to consume shuffle data, but the condition is not sufficient, this PR updates the logic to match `org.apache.spark.network.client.TransportClientFactory` ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Manual testing. Closes #38982 from pan3793/SPARK-41376-3.2. Authored-by: Cheng Pan <chengpan@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

… executor start ### What changes were proposed in this pull request? Fix the condition for judging Netty prefer direct memory on executor start, the logic should match `org.apache.spark.network.client.TransportClientFactory`. ### Why are the changes needed? The check logical was added in SPARK-27991, the original intention is to avoid potential Netty OOM issue when Netty uses direct memory to consume shuffle data, but the condition is not sufficient, this PR updates the logic to match `org.apache.spark.network.client.TransportClientFactory` ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Manual testing. Closes apache#38901 from pan3793/SPARK-41376. Authored-by: Cheng Pan <chengpan@apache.org> Signed-off-by: Sean Owen <srowen@gmail.com>

…ic on executor start ### What changes were proposed in this pull request? Backport apache#38901 to branch-3.2. Fix the condition for judging Netty prefer direct memory on executor start, the logic should match `org.apache.spark.network.client.TransportClientFactory`. ### Why are the changes needed? The check logical was added in SPARK-27991, the original intention is to avoid potential Netty OOM issue when Netty uses direct memory to consume shuffle data, but the condition is not sufficient, this PR updates the logic to match `org.apache.spark.network.client.TransportClientFactory` ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Manual testing. Closes apache#38982 from pan3793/SPARK-41376-3.2. Authored-by: Cheng Pan <chengpan@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

[SPARK-41376][CORE] Executor netty direct memory check should respect…

9def9fc

… spark.shuffle.io.preferDirectBufs

github-actions bot added the CORE label Dec 4, 2022

srowen reviewed Dec 4, 2022

View reviewed changes

core/src/main/scala/org/apache/spark/executor/CoarseGrainedExecutorBackend.scala Outdated Show resolved Hide resolved

address

91515df

pan3793 changed the title ~~[SPARK-41376][CORE] Executor netty direct memory check should respect spark.shuffle.io.preferDirectBufs~~ [SPARK-41376][CORE] Correct the Netty preferDirectBufs check logic on executor start Dec 5, 2022

Ngone51 reviewed Dec 5, 2022

View reviewed changes

core/src/main/scala/org/apache/spark/executor/CoarseGrainedExecutorBackend.scala Outdated Show resolved Hide resolved

Ngone51 reviewed Dec 6, 2022

View reviewed changes

pan3793 added 2 commits December 7, 2022 10:47

address comment

5fc5080

nit

e1198f4

LuciferYang approved these changes Dec 7, 2022

View reviewed changes

srowen reviewed Dec 7, 2022

View reviewed changes

srowen closed this in e6be300 Dec 8, 2022

This was referenced Dec 8, 2022

[SPARK-41376][CORE][3.3] Correct the Netty preferDirectBufs check logic on executor start #38981

Closed

[SPARK-41376][CORE][3.2] Correct the Netty preferDirectBufs check logic on executor start #38982

Closed

pan3793 mentioned this pull request Mar 31, 2023

[Bug][Helm] Vanilla Helm deployment apache/kyuubi#4629

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-41376][CORE] Correct the Netty preferDirectBufs check logic on executor start #38901

[SPARK-41376][CORE] Correct the Netty preferDirectBufs check logic on executor start #38901

pan3793 commented Dec 4, 2022 •

edited

Loading

pan3793 commented Dec 4, 2022

AmplabJenkins commented Dec 4, 2022

pan3793 commented Dec 5, 2022

srowen commented Dec 5, 2022

mridulm commented Dec 5, 2022

Ngone51 Dec 6, 2022

pan3793 Dec 6, 2022

Ngone51 Dec 6, 2022 •

edited

Loading

pan3793 Dec 6, 2022

LuciferYang Dec 6, 2022

pan3793 Dec 7, 2022

LuciferYang left a comment

srowen left a comment

pan3793 commented Dec 7, 2022 •

edited

Loading

srowen commented Dec 8, 2022

dongjoon-hyun commented Dec 8, 2022

pan3793 commented Dec 8, 2022

dongjoon-hyun commented Dec 8, 2022

pan3793 commented Dec 8, 2022

[SPARK-41376][CORE] Correct the Netty preferDirectBufs check logic on executor start #38901

[SPARK-41376][CORE] Correct the Netty preferDirectBufs check logic on executor start #38901

Conversation

pan3793 commented Dec 4, 2022 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

pan3793 commented Dec 4, 2022

AmplabJenkins commented Dec 4, 2022

pan3793 commented Dec 5, 2022

srowen commented Dec 5, 2022

mridulm commented Dec 5, 2022

Ngone51 Dec 6, 2022

Choose a reason for hiding this comment

pan3793 Dec 6, 2022

Choose a reason for hiding this comment

Ngone51 Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

pan3793 Dec 6, 2022

Choose a reason for hiding this comment

LuciferYang Dec 6, 2022

Choose a reason for hiding this comment

pan3793 Dec 7, 2022

Choose a reason for hiding this comment

LuciferYang left a comment

Choose a reason for hiding this comment

srowen left a comment

Choose a reason for hiding this comment

pan3793 commented Dec 7, 2022 • edited Loading

srowen commented Dec 8, 2022

dongjoon-hyun commented Dec 8, 2022

pan3793 commented Dec 8, 2022

dongjoon-hyun commented Dec 8, 2022

pan3793 commented Dec 8, 2022

pan3793 commented Dec 4, 2022 •

edited

Loading

Ngone51 Dec 6, 2022 •

edited

Loading

pan3793 commented Dec 7, 2022 •

edited

Loading