[SPARK-13776][WebUI]Limit the max number of acceptors and selectors for Jetty #11615
Conversation
zsxwing
changed the title
|
Another option is just hard-core the number of acceptors to a small number. I don't think there will be a lot of connections to access the Web UI at the same time. |
|
Test build #52768 has finished for PR 11615 at commit
|
| server.setConnectors(connectors.toArray) | ||
|
|
||
| val pool = new QueuedThreadPool | ||
| var maxThreads = conf.getInt("spark.ui.threads", pool.getMaxThreads) |
There was a problem hiding this comment.
Do we even need config here? when would I otherwise want to control this? It seems like you want to always make the pool have at least acceptors+1 threads.
There was a problem hiding this comment.
I was worried that 1 thread for processing requests may be not enough. Then people have a configuration to increase it.
@srowen what do you think about just hard-core the max number of acceptors to a small number? Maybe just 8?
The default configuration of Jetty seems good to a web application that supports massive connections. However, Spark Web UI doesn't need to support so many users, so it's a waste to create many threads.
There was a problem hiding this comment.
Agree, if it's just for web UIs then I see no need for a large number of acceptors. I think your change is still needed to make sure the pool has enough threads in any event? though maybe the config is unnecessary.
|
Updated to use a small acceptor number |
| // As each Acceptor will use one thread, the number of threads should at least be the number | ||
| // of acceptors plus 1. (See SPARK-13776) | ||
| var minThreads = 1 | ||
| connectors.collect { case c: AbstractConnector => c }.foreach { c => |
There was a problem hiding this comment.
Nit: the Scala math class is math vs Math but I wouldn't change it just for that. I think this looks like the right kind of approach.
|
Test build #52848 has finished for PR 11615 at commit
|
| val connector = c.asInstanceOf[SelectChannelConnector] | ||
| // Limit the max acceptor number to 8 so that we don't waste a lot of threads | ||
| connector.setAcceptors(math.min(connector.getAcceptors, 8)) | ||
| // The number of selectors always equals to the number of acceptors |
There was a problem hiding this comment.
|
Test build #52872 has finished for PR 11615 at commit
|
|
retest this please |
|
Test build #52908 has finished for PR 11615 at commit
|
|
retest this please |
zsxwing
changed the title
| // As each acceptor and each selector will use one thread, the number of threads should at | ||
| // least be the number of acceptors and selectors plus 1. (See SPARK-13776) | ||
| var minThreads = 1 | ||
| connectors.foreach { c => |
There was a problem hiding this comment.
Use collect or "cast" using pattern matching on type.
There was a problem hiding this comment.
I want to crash the app if we add a non-SelectChannelConnector in future. It's better than being silent.
|
Test build #52931 has finished for PR 11615 at commit
|
|
Test build #52949 has finished for PR 11615 at commit
|
|
@srowen what do you think now? |
| @@ -134,7 +134,7 @@ private[spark] abstract class WebUI( | |||
| def bind() { | |||
| assert(!serverInfo.isDefined, "Attempted to bind %s more than once!".format(className)) | |||
| try { | |||
| var host = Option(conf.getenv("SPARK_LOCAL_IP")).getOrElse("0.0.0.0") | |||
| val host = Option(conf.getenv("SPARK_LOCAL_IP")).getOrElse("0.0.0.0") | |||
There was a problem hiding this comment.
OK but was this change intentional? rest seems OK
There was a problem hiding this comment.
Oh, never mind. I will remove it.
|
Test build #53221 has finished for PR 11615 at commit
|
|
retest this please |
|
Test build #53232 has finished for PR 11615 at commit
|
|
retest this please |
|
Test build #53249 has finished for PR 11615 at commit
|
|
LGTM, and the original reporter confirmed it works. I'm merging to master. I'm open to an argument that this should go into 1.6 though the scope of the problem is quite limited, so maybe isn't worth the subtle behavior change. |
|
Agree that this doesn't need to go into 1.6. |
…for Jetty ## What changes were proposed in this pull request? As each acceptor/selector in Jetty will use one thread, the number of threads should at least be the number of acceptors and selectors plus 1. Otherwise, the thread pool of Jetty server may be exhausted by acceptors/selectors and not be able to response any request. To avoid wasting threads, the PR limits the max number of acceptors and selectors and also updates the max thread number if necessary. ## How was this patch tested? Just make sure we don't break any existing tests Author: Shixiong Zhu <[email protected]> Closes apache#11615 from zsxwing/SPARK-13776.
What changes were proposed in this pull request?
As each acceptor/selector in Jetty will use one thread, the number of threads should at least be the number of acceptors and selectors plus 1. Otherwise, the thread pool of Jetty server may be exhausted by acceptors/selectors and not be able to response any request.
To avoid wasting threads, the PR limits the max number of acceptors and selectors and also updates the max thread number if necessary.
How was this patch tested?
Just make sure we don't break any existing tests