[SPARK-21176] [Web UI] Limit number of selector threads for admin ui proxy servlets to 8 #18437
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Test build #3815 has finished for PR 18437 at commit
|
srowen
reviewed
Jun 28, 2017
| override def newHttpClient(): HttpClient = { | ||
| // Use the Jetty logic to calculate the number of selector threads (#CPUs/2), | ||
| // but limit it to 8 max. | ||
| val numSelectors = math.max(1,math.min(8,Runtime.getRuntime().availableProcessors()/2)) |
Member
There was a problem hiding this comment.
This will fail style tests @IngoSchuster -- we need spaces around operators, after commas, etc.
Contributor
Author
There was a problem hiding this comment.
Aargh - what an unnecessary failure, sorry about that :(.
I've corrected the line in question, the Scala style tests do pass now.
srowen
reviewed
Jun 29, 2017
| override def newHttpClient(): HttpClient = { | ||
| // Use the Jetty logic to calculate the number of selector threads (#CPUs/2), | ||
| // but limit it to 8 max. | ||
| val numSelectors = math.max(1, math.min(8, Runtime.getRuntime().availableProcessors()/2)) |
Member
There was a problem hiding this comment.
I think you still need spaces around /, and omit return on the next line
Contributor
Author
There was a problem hiding this comment.
Ok, I changed that as well. The run-tests scala style checks pass with both the latest and the previous version.
jiangxb1987
approved these changes
Jun 29, 2017
|
|
||
| override def newHttpClient(): HttpClient = { | ||
| // Use the Jetty logic to calculate the number of selector threads (#CPUs/2), | ||
| // but limit it to 8 max. |
Contributor
There was a problem hiding this comment.
Could you also explain why we are limiting the number of selectors in the comment above?
Contributor
|
retest this please |
Contributor
|
retest this please |
|
Test build #78942 has finished for PR 18437 at commit
|
asfgit
pushed a commit
that referenced
this pull request
Jun 30, 2017
…roxy servlets to 8 ## What changes were proposed in this pull request? Please see also https://issues.apache.org/jira/browse/SPARK-21176 This change limits the number of selector threads that jetty creates to maximum 8 per proxy servlet (Jetty default is number of processors / 2). The newHttpClient for Jettys ProxyServlet class is overwritten to avoid the Jetty defaults (which are designed for high-performance http servers). Once jetty/jetty.project#1643 is available, the code could be cleaned up to avoid the method override. I really need this on v2.1.1 - what is the best way for a backport automatic merge works fine)? Shall I create another PR? ## How was this patch tested? (Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests) The patch was tested manually on a Spark cluster with a head node that has 88 processors using JMX to verify that the number of selector threads is now limited to 8 per proxy. gurvindersingh zsxwing can you please review the change? Author: IngoSchuster <[email protected]> Author: Ingo Schuster <[email protected]> Closes #18437 from IngoSchuster/master. (cherry picked from commit 88a536b) Signed-off-by: Wenchen Fan <[email protected]>
Contributor
|
LGTM, merging to maser/2.2/2.1! |
asfgit
pushed a commit
that referenced
this pull request
Jun 30, 2017
…roxy servlets to 8 ## What changes were proposed in this pull request? Please see also https://issues.apache.org/jira/browse/SPARK-21176 This change limits the number of selector threads that jetty creates to maximum 8 per proxy servlet (Jetty default is number of processors / 2). The newHttpClient for Jettys ProxyServlet class is overwritten to avoid the Jetty defaults (which are designed for high-performance http servers). Once jetty/jetty.project#1643 is available, the code could be cleaned up to avoid the method override. I really need this on v2.1.1 - what is the best way for a backport automatic merge works fine)? Shall I create another PR? ## How was this patch tested? (Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests) The patch was tested manually on a Spark cluster with a head node that has 88 processors using JMX to verify that the number of selector threads is now limited to 8 per proxy. gurvindersingh zsxwing can you please review the change? Author: IngoSchuster <[email protected]> Author: Ingo Schuster <[email protected]> Closes #18437 from IngoSchuster/master. (cherry picked from commit 88a536b) Signed-off-by: Wenchen Fan <[email protected]>
Contributor
Author
|
Thank you all for your support! |
ghost
pushed a commit
to dbtsai/spark
that referenced
this pull request
Aug 9, 2017
…and applications ## What changes were proposed in this pull request? Currently, each application and each worker creates their own proxy servlet. Each proxy servlet is backed by its own HTTP client and a relatively large number of selector threads. This is excessive but was fixed (to an extent) by apache#18437. However, a single HTTP client (backed by a single selector thread) should be enough to handle all proxy requests. This PR creates a single proxy servlet no matter how many applications and workers there are. ## How was this patch tested? . The unit tests for rewriting proxied locations and headers were updated. I then spun up a 100 node cluster to ensure that proxy'ing worked correctly jiangxb1987 Please let me know if there's anything else I can do to help push this thru. Thanks! Author: Anderson Osagie <[email protected]> Closes apache#18499 from aosagie/fix/minimize-proxy-threads.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Please see also https://issues.apache.org/jira/browse/SPARK-21176
This change limits the number of selector threads that jetty creates to maximum 8 per proxy servlet (Jetty default is number of processors / 2).
The newHttpClient for Jettys ProxyServlet class is overwritten to avoid the Jetty defaults (which are designed for high-performance http servers).
Once jetty/jetty.project#1643 is available, the code could be cleaned up to avoid the method override.
I really need this on v2.1.1 - what is the best way for a backport automatic merge works fine)? Shall I create another PR?
How was this patch tested?
(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)
The patch was tested manually on a Spark cluster with a head node that has 88 processors using JMX to verify that the number of selector threads is now limited to 8 per proxy.
@gurvindersingh @zsxwing can you please review the change?