[SPARK-25692] [CORE] Remove static initialization of worker eventLoop handling chunk fetch requests within TransportContext. This fixes ChunkFetchIntegrationSuite as well #23700

redsanket · 2019-01-30T15:23:36Z

What changes were proposed in this pull request?

How to reproduce
./build/mvn test -Dtest=org.apache.spark.network.RequestTimeoutIntegrationSuite,org.apache.spark.network.ChunkFetchIntegrationSuite -DwildcardSuites=None test
furtherRequestsDelay Test within RequestTimeoutIntegrationSuite was holding onto buffer references within worker threads. The test does close the server context but since the threads are global and there is sleep of 60 secs to fetch a specific chunk within this test, it grabs on it and waits for the client to consume but however the test is testing for a request timeout and it times out after 10 secs, so the workers are just waiting there for the buffer to be consumed by client as per my understanding.

This tends to happen if you dont have enough IO threads available on the specific system and also the order of the tests being run determines its flakyness like if ChunkFetchIntegrationSuite runs first then there is no issue. For example on mac with 8 threads these tests run fine but on my vm with 4 threads it fails. It matches the number of fetch calls in RequestTimeoutIntegrationSuite.

So do we really need it to be static?

I dont think this requires a global declaration as these threads are only required on the shuffle server end and on the client TransportContext initialization i.e the Client don't initialize these threads. The Shuffle Server initializes one TransportContext object. So, I think this is fine to be an instance variable and I see no harm.

How was this patch tested?

Integration tests, manual tests

redsanket · 2019-01-30T15:24:09Z

@zsxwing @dongjoon-hyun Plz take a look as you have context on this already thanks

tgravescs · 2019-01-30T16:59:11Z

ok to test

dongjoon-hyun · 2019-01-30T20:12:32Z

Retest this please.

dongjoon-hyun · 2019-01-30T20:15:15Z

The current run is on amp-jenkins-worker-04. I re-triggered this one to get a chance to run this PR on another machine, hopefully amp-jenkins-worker-05.

Currently, #23704 is suffering from ChunkFetchIntegrationSuite on amp-jenkins-worker-05.

zsxwing

LGTM except one nit.

In addition, I noticed that we don't have any flag for this new feature. This is the first time we add it and it could have some bugs. Could you add it in another PR?

dongjoon-hyun · 2019-01-30T20:54:04Z

~~Hi, @zsxwing . Could you merge this PR?~~ Oh. Never mind. It's still on Jenkins.

redsanket · 2019-01-30T21:16:42Z

@zsxwing you could always disable this by setting spark.shuffle.server.chunkFetchHandlerThreadsPercent to 100. This is turned off by default. To use this feature you have to reduce spark.shuffle.server.chunkFetchHandlerThreadsPercent to like say for example 80 which says ok i have 10 IO threads use only 8 for chunk fetch requests rest for other rpc calls. This is wrt shuffle server

zsxwing · 2019-01-30T21:33:01Z

@redsanket IIUC, all chunks are always handled in a separate event loop and there is no way to change it back to the original single-thread mode. Right?

redsanket · 2019-01-30T21:48:14Z

right they are handled separately correct but the behavior remains same @zsxwing. We are running our production clusters and haven't seen stability issues yet

SparkQA · 2019-01-30T21:52:55Z

Test build #101899 has finished for PR 23700 at commit 60889d2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

redsanket · 2019-01-30T22:16:37Z

Retest this please

SparkQA · 2019-01-31T01:03:07Z

Test build #101912 has finished for PR 23700 at commit 60889d2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen

I don't see that there is a behavior change here regarding event loops vs single thread?

But the constructor needs to be updated now; it doesn't need to synchronize on the class to initialize this.

The PR title needs to be more descriptive too.

srowen

(oops meant to click "Request Changes")

redsanket · 2019-01-31T13:00:12Z

Nice catch will update thanks @srowen

…ialization

redsanket · 2019-01-31T13:35:01Z

Retest this please

srowen · 2019-01-31T13:41:37Z

common/network-common/src/main/java/org/apache/spark/network/TransportContext.java

-            conf.chunkFetchHandlerThreads(),
-            "shuffle-chunk-fetch-handler");
-      }
+    if (chunkFetchWorkers == null &&


No need to check for null; it always is here

yes i think this was more of a defensive check when static was around will remove thanks

srowen · 2019-01-31T13:42:12Z

common/network-common/src/main/java/org/apache/spark/network/TransportContext.java

@@ -88,7 +88,7 @@
  // Separate thread pool for handling ChunkFetchRequest. This helps to enable throttling
  // max number of TransportServer worker threads that are blocked on writing response
  // of ChunkFetchRequest message back to the client via the underlying channel.
-  private static EventLoopGroup chunkFetchWorkers;
+  private EventLoopGroup chunkFetchWorkers;


This can be final, though will require explicitly setting it to null in the constructor if not otherwise initialized

yes makes sense to make it final thanks will do

redsanket · 2019-01-31T14:25:41Z

Retest this please

SparkQA · 2019-01-31T17:35:59Z

Test build #101952 has finished for PR 23700 at commit 657dff2.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-01-31T19:25:18Z

Test build #101954 has finished for PR 23700 at commit 3f0eb60.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

redsanket · 2019-02-01T14:02:26Z

gentle ping @srowen @zsxwing @tgravescs @dongjoon-hyun thanks

tgravescs · 2019-02-03T16:25:24Z

Removing the static seems fine here.

I am curious if you put in an extra sleep(60) at the end of furtherRequestsDelay, that works around it as well? Not advocating that as fix just making sure I understand and nothing else is going on.

redsanket · 2019-02-04T15:20:12Z

@tgravescs yes that works too... tested that as well but i think removing static is for the best.

srowen · 2019-02-05T18:45:02Z

Merged to master

…handling chunk fetch requests within TransportContext. This fixes ChunkFetchIntegrationSuite as well ## What changes were proposed in this pull request? How to reproduce ./build/mvn test -Dtest=org.apache.spark.network.RequestTimeoutIntegrationSuite,org.apache.spark.network.ChunkFetchIntegrationSuite -DwildcardSuites=None test furtherRequestsDelay Test within RequestTimeoutIntegrationSuite was holding onto buffer references within worker threads. The test does close the server context but since the threads are global and there is sleep of 60 secs to fetch a specific chunk within this test, it grabs on it and waits for the client to consume but however the test is testing for a request timeout and it times out after 10 secs, so the workers are just waiting there for the buffer to be consumed by client as per my understanding. This tends to happen if you dont have enough IO threads available on the specific system and also the order of the tests being run determines its flakyness like if ChunkFetchIntegrationSuite runs first then there is no issue. For example on mac with 8 threads these tests run fine but on my vm with 4 threads it fails. It matches the number of fetch calls in RequestTimeoutIntegrationSuite. So do we really need it to be static? I dont think this requires a global declaration as these threads are only required on the shuffle server end and on the client TransportContext initialization i.e the Client don't initialize these threads. The Shuffle Server initializes one TransportContext object. So, I think this is fine to be an instance variable and I see no harm. ## How was this patch tested? Integration tests, manual tests Closes apache#23700 from redsanket/SPARK-25692. Authored-by: schintap <[email protected]> Signed-off-by: Sean Owen <[email protected]>

…handling chunk fetch requests within TransportContext. This fixes ChunkFetchIntegrationSuite as well How to reproduce ./build/mvn test -Dtest=org.apache.spark.network.RequestTimeoutIntegrationSuite,org.apache.spark.network.ChunkFetchIntegrationSuite -DwildcardSuites=None test furtherRequestsDelay Test within RequestTimeoutIntegrationSuite was holding onto buffer references within worker threads. The test does close the server context but since the threads are global and there is sleep of 60 secs to fetch a specific chunk within this test, it grabs on it and waits for the client to consume but however the test is testing for a request timeout and it times out after 10 secs, so the workers are just waiting there for the buffer to be consumed by client as per my understanding. This tends to happen if you dont have enough IO threads available on the specific system and also the order of the tests being run determines its flakyness like if ChunkFetchIntegrationSuite runs first then there is no issue. For example on mac with 8 threads these tests run fine but on my vm with 4 threads it fails. It matches the number of fetch calls in RequestTimeoutIntegrationSuite. So do we really need it to be static? I dont think this requires a global declaration as these threads are only required on the shuffle server end and on the client TransportContext initialization i.e the Client don't initialize these threads. The Shuffle Server initializes one TransportContext object. So, I think this is fine to be an instance variable and I see no harm. Integration tests, manual tests Closes apache#23700 from redsanket/SPARK-25692. Authored-by: schintap <[email protected]> Signed-off-by: Sean Owen <[email protected]> (cherry picked from commit 13c5634) (cherry picked from commit 0957414d520225ed08dfdcaf383d9c825a969adc) Change-Id: I7bb43d650915e3c99aba79b2f1183a72b4308985 (cherry picked from commit 02a2150957bf3ab167d69c00c3563cffb3ed005e)

Fix ChunkFetchIntegrationSuite

60889d2

redsanket changed the title ~~[SPARK-25692] Fix ChunkFetchIntegrationSuite~~ [SPARK-25692] [core] Fix ChunkFetchIntegrationSuite Jan 30, 2019

redsanket changed the title ~~[SPARK-25692] [core] Fix ChunkFetchIntegrationSuite~~ [SPARK-25692] [CORE] Fix ChunkFetchIntegrationSuite Jan 30, 2019

zsxwing reviewed Jan 30, 2019

View reviewed changes

dongjoon-hyun mentioned this pull request Jan 30, 2019

[SPARK-26677][BUILD] Update Parquet to 1.10.1 with notEq pushdown fix. #23704

Closed

srowen approved these changes Jan 31, 2019

View reviewed changes

srowen requested changes Jan 31, 2019

View reviewed changes

redsanket changed the title ~~[SPARK-25692] [CORE] Fix ChunkFetchIntegrationSuite~~ [SPARK-25692] [CORE] Remove static initialization of worker eventLoop handling chunk fetch requests within TransportContext. This fixes ChunkFetchIntegrationSuite Jan 31, 2019

Remove synchronize block around shuffle server transport context init…

657dff2

…ialization

srowen reviewed Jan 31, 2019

View reviewed changes

Make event loop group final

3f0eb60

srowen approved these changes Feb 2, 2019

View reviewed changes

srowen closed this in 13c5634 Feb 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-25692] [CORE] Remove static initialization of worker eventLoop handling chunk fetch requests within TransportContext. This fixes ChunkFetchIntegrationSuite as well #23700

[SPARK-25692] [CORE] Remove static initialization of worker eventLoop handling chunk fetch requests within TransportContext. This fixes ChunkFetchIntegrationSuite as well #23700

redsanket commented Jan 30, 2019 •

edited by dongjoon-hyun

Loading

redsanket commented Jan 30, 2019

tgravescs commented Jan 30, 2019

dongjoon-hyun commented Jan 30, 2019

dongjoon-hyun commented Jan 30, 2019

zsxwing left a comment •

edited

Loading

dongjoon-hyun commented Jan 30, 2019 •

edited

Loading

redsanket commented Jan 30, 2019 •

edited

Loading

zsxwing commented Jan 30, 2019

redsanket commented Jan 30, 2019 •

edited

Loading

SparkQA commented Jan 30, 2019

redsanket commented Jan 30, 2019

SparkQA commented Jan 31, 2019

srowen left a comment

srowen left a comment

redsanket commented Jan 31, 2019

redsanket commented Jan 31, 2019

srowen Jan 31, 2019

redsanket Jan 31, 2019

srowen Jan 31, 2019

redsanket Jan 31, 2019

redsanket commented Jan 31, 2019

SparkQA commented Jan 31, 2019

SparkQA commented Jan 31, 2019

redsanket commented Feb 1, 2019

tgravescs commented Feb 3, 2019

redsanket commented Feb 4, 2019

srowen commented Feb 5, 2019

[SPARK-25692] [CORE] Remove static initialization of worker eventLoop handling chunk fetch requests within TransportContext. This fixes ChunkFetchIntegrationSuite as well #23700

[SPARK-25692] [CORE] Remove static initialization of worker eventLoop handling chunk fetch requests within TransportContext. This fixes ChunkFetchIntegrationSuite as well #23700

Conversation

redsanket commented Jan 30, 2019 • edited by dongjoon-hyun Loading

What changes were proposed in this pull request?

How was this patch tested?

redsanket commented Jan 30, 2019

tgravescs commented Jan 30, 2019

dongjoon-hyun commented Jan 30, 2019

dongjoon-hyun commented Jan 30, 2019

zsxwing left a comment • edited Loading

Choose a reason for hiding this comment

dongjoon-hyun commented Jan 30, 2019 • edited Loading

redsanket commented Jan 30, 2019 • edited Loading

zsxwing commented Jan 30, 2019

redsanket commented Jan 30, 2019 • edited Loading

SparkQA commented Jan 30, 2019

redsanket commented Jan 30, 2019

SparkQA commented Jan 31, 2019

srowen left a comment

Choose a reason for hiding this comment

srowen left a comment

Choose a reason for hiding this comment

redsanket commented Jan 31, 2019

redsanket commented Jan 31, 2019

srowen Jan 31, 2019

Choose a reason for hiding this comment

redsanket Jan 31, 2019

Choose a reason for hiding this comment

srowen Jan 31, 2019

Choose a reason for hiding this comment

redsanket Jan 31, 2019

Choose a reason for hiding this comment

redsanket commented Jan 31, 2019

SparkQA commented Jan 31, 2019

SparkQA commented Jan 31, 2019

redsanket commented Feb 1, 2019

tgravescs commented Feb 3, 2019

redsanket commented Feb 4, 2019

srowen commented Feb 5, 2019

redsanket commented Jan 30, 2019 •

edited by dongjoon-hyun

Loading

zsxwing left a comment •

edited

Loading

dongjoon-hyun commented Jan 30, 2019 •

edited

Loading

redsanket commented Jan 30, 2019 •

edited

Loading

redsanket commented Jan 30, 2019 •

edited

Loading