-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-25641] Change the spark.shuffle.server.chunkFetchHandlerThreadsPercent default to 100 #22628
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@tgravescs @abellina plz take a look thanks |
|
ok to test |
abellina
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 pending tests
|
Test build #96943 has finished for PR 22628 at commit
|
|
Test build #96951 has finished for PR 22628 at commit
|
|
+1 |
|
I'm not clear from the description what the issue is. I get that the number of threads is undesirable but can you illustrate with a clearer example? |
|
the default of 0 gets you the netty default which is always 2 * the # of cores. It ignores if you set the io.serverThreads to set the # of shuffle threads. With a default of 100, it actually applies the setting for io.serverThreads or if you don't have it set then uses the 2 * # of cores. So having a default of 100 is better here. io.serverThreads = 10 and spark.shuffle.server.chunkFetchHandlerThreadsPercent=100 then the # of threads here for chunked fetches is 10 Its better to have a default of 100 to keep the current behavior. |
|
Thanks @tgravescs for explaining the issue |
|
pulled into master, thanks @redsanket |
|
Is it supposed to the flaky ChunkFetchIntegrationSuite? http://spark-tests.appspot.com/test-details?suite_name=org.apache.spark.network.ChunkFetchIntegrationSuite&test_name=fetchFileChunk |
|
not sure what you are asking, if you are saying its flaky then file a jira and please cc me and Sanket. The link above doesn't load for me, please give me permissions if its needed. |
…sPercent default to 100 ## What changes were proposed in this pull request? We want to change the default percentage to 100 for spark.shuffle.server.chunkFetchHandlerThreadsPercent. The reason being currently this is set to 0. Which means currently if server.ioThreads > 0, the default number of threads would be 2 * #cores instead of server.io.Threads. We want the default to server.io.Threads in case this is not set at all. Also here a default of 0 would also mean 2 * #cores ## How was this patch tested? Manual (Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests) (If this patch involves UI changes, please attach a screenshot; otherwise, remove this) Please review http://spark.apache.org/contributing.html before opening a pull request. Closes apache#22628 from redsanket/SPARK-25641. Lead-authored-by: Sanket Chintapalli <[email protected]> Co-authored-by: Sanket Chintapalli <[email protected]> Signed-off-by: Thomas Graves <[email protected]>
…sPercent default to 100 We want to change the default percentage to 100 for spark.shuffle.server.chunkFetchHandlerThreadsPercent. The reason being currently this is set to 0. Which means currently if server.ioThreads > 0, the default number of threads would be 2 * #cores instead of server.io.Threads. We want the default to server.io.Threads in case this is not set at all. Also here a default of 0 would also mean 2 * #cores Manual (Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests) (If this patch involves UI changes, please attach a screenshot; otherwise, remove this) Please review http://spark.apache.org/contributing.html before opening a pull request. Closes apache#22628 from redsanket/SPARK-25641. Lead-authored-by: Sanket Chintapalli <[email protected]> Co-authored-by: Sanket Chintapalli <[email protected]> Signed-off-by: Thomas Graves <[email protected]> (cherry picked from commit 6353425) (cherry picked from commit b4cc97a3579ac01061b8c3fce52ea16542aa07e4) Change-Id: If487d8ac1366933bc3eb4fe9822c586f23081904 (cherry picked from commit e3b58210eb1c5c2a9a040a7b96305043c927bfbf)
What changes were proposed in this pull request?
We want to change the default percentage to 100 for spark.shuffle.server.chunkFetchHandlerThreadsPercent. The reason being
currently this is set to 0. Which means currently if server.ioThreads > 0, the default number of threads would be 2 * #cores instead of server.io.Threads. We want the default to server.io.Threads in case this is not set at all. Also here a default of 0 would also mean 2 * #cores
How was this patch tested?
Manual
(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)
Please review http://spark.apache.org/contributing.html before opening a pull request.