[SPARK-21176][WEB UI] Use a single ProxyServlet to proxy all workers and applications #18499
Conversation
|
OK to test |
|
Hi @cloud-fan, |
|
Test build #3831 has finished for PR 18499 at commit
|
|
cc @jiangxb1987 |
|
cc @gurvindersingh Could you look at this please? |
|
cc @sarutak @gengliangwang Could you look at this when you have time? Thanks! |
|
Surely math.max is correct? or else the value is always 1.
…On Tue, Jul 4, 2017 at 3:28 PM IngoSchuster ***@***.***> wrote:
Can we pull that also into 2.1.1? The original code in JettyUtils to limit
the number of selector threads is actually wrong.
Instead of
val numSelectors = math.max(1, math.min(8,
Runtime.getRuntime().availableProcessors() / 2))
it should have been
val numSelectors = math.min(1, math.max(8,
Runtime.getRuntime().availableProcessors() / 2))
With this PR, a fix for the bug above is no longer needed.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#18499 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAyM-hHnlELpIMQhDoh54FOG1XS5_DxCks5sKkvwgaJpZM4OLSCA>
.
|
There was a problem hiding this comment.
In my opinion, it's a bit extreme to just have a single selector thread. Browsers fetch elements of a page concurrently and it will help for a more responsive user experience if we serve them in parallel.
When I reload the admin ui page, it seems as if 10+ requests are sent concurrently. For the sake of web ui latency I think we could be a bit more generous with selector threads - in particular with this great improvement that reduces the number of proxy servlets to just 1.
There was a problem hiding this comment.
Thanks for the feedback @IngoSchuster
I can go with whichever number people find best. My main goal with this patch was to not have that number grow with the number of workers and applications (since my org runs a lot of applications and ran into issues with Master not being responsive).
From my research, I found the following quote from a Jetty committer: "For 5k clients for normal HTML pages you can easily go by with 1 selector." (See: https://dev.eclipse.org/mhonarc/lists/jetty-users/msg04751.html). But, this was in the context of a server and not necessarily a client.
There was a problem hiding this comment.
I fully agree @aosagie - it's much better if we don't scale the number of selector threads up with the number of executors.
But since we don't serve simple html pages but also page(elements) that might have a higher latency than simply serving static files, I think we should not risk that the page is build up from serial requests where latency adds up - my server for example has 170ms network latency.
I suggest that we set the number of selector threads to 8. This is certainly not excessive but still allows to serve a page with several elements in parallel.
|
You are right, a short moment of confusion - already deleted my comment...
;-)
Please disregard it
Regards,
Ingo Schuster
Tel: +49-(0)7031-16-5156
IBM Analytics
----------------------------------------------------------------------------
IBM Deutschland Research & Development GmbH / Vorsitzende des
Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart,
HRB 243294
From: Sean Owen <[email protected]>
To: apache/spark <[email protected]>
Cc: IngoSchuster <[email protected]>, Comment
<[email protected]>
Date: 04/07/2017 17:11
Subject: Re: [apache/spark] [SPARK-21176][WEB UI] Use a single
ProxyServlet to proxy all workers and applications (#18499)
Surely math.max is correct? or else the value is always 1.
On Tue, Jul 4, 2017 at 3:28 PM IngoSchuster ***@***.***> wrote:
Can we pull that also into 2.1.1? The original code in JettyUtils to
limit
the number of selector threads is actually wrong.
Instead of
val numSelectors = math.max(1, math.min(8,
Runtime.getRuntime().availableProcessors() / 2))
it should have been
val numSelectors = math.min(1, math.max(8,
Runtime.getRuntime().availableProcessors() / 2))
With this PR, a fix for the bug above is no longer needed.
?
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#18499 (comment)>, or
mute
the thread
<
.
?
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.
|
aosagie
force-pushed
the
fix/minimize-proxy-threads
branch
from
2b907df to
ef3048c
Compare
|
@cloud-fan Hi, I just pushed a change to up the selector threads from 1 to 8. Can I get a retest please? @jiangxb1987 Sorry to bother, but is there anyone available to give guidance on what I need to do to push this PR forward? This bug renders the master completely unresponsive for us in production. |
|
Hey @ajbozarth. Any chance you could provide a review or some guidance on anything I can do to make this PR more amenable? |
|
retest this please |
There was a problem hiding this comment.
Could you add comments for the above code?
There was a problem hiding this comment.
I think we can just keep the previous code here. If the number of available processors is smaller than 8, e.g. only one processor, we don't need to make it 8 threads.
There was a problem hiding this comment.
I think we can save use a variable to save the second parameter.
|
retest this please |
|
Test build #80216 has finished for PR 18499 at commit
|
aosagie
force-pushed
the
fix/minimize-proxy-threads
branch
2 times, most recently
from
f998727 to
104c54a
Compare
aosagie
force-pushed
the
fix/minimize-proxy-threads
branch
from
104c54a to
70c93b0
Compare
|
retest this please |
|
I don't see any code issues, but I'm confident enough in my proxy knowledge to give this a "lgtm" |
|
Test build #80355 has finished for PR 18499 at commit
|
|
thanks, merging to master! |
|
@aosagie do we still need the previous fix for SPARK-21176? |
|
Yes I believe it still makes sense to limit the number of selector threads. Jetty's default is tuned for a real web server and by default it creates (number of cpus / 2) selector threads. Right now we have limited the number of threads to 8 which I believe is more than enough to serve the spark admin UI. |
|
Yes, I agree with @IngoSchuster |
9 participants
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 #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!