[SPARK-22914][DEPLOY] Register history.ui.port #20098
Conversation
|
CC @vanzin |
vanzin
left a comment
vanzin
left a comment
There was a problem hiding this comment.
Idea sounds ok, although it requires the client configuration to also contain history server configuration to properly work, which may not be the case in the most common case.
It needs to be rebased, though.
There was a problem hiding this comment.
Just add this to the ApplicationMaster object instead of creating yet another utils object... and also add private[spark] to the method there.
There was a problem hiding this comment.
ResetSystemProperties not needed since you're not changing system properties.
There was a problem hiding this comment.
Logging not needed, you're not logging anything.
There was a problem hiding this comment.
Use assert(foo === bar) (and then you also don't need Matchers).
Also, better to use interpolation to check. e.g. foo === s"http://$host:$post/history/$appId/$attemptId". You can set the history port in SparkConf to avoid having to read the default value in some other way.
There was a problem hiding this comment.
These imports will fail style checks. Make sure to run them locally.
There was a problem hiding this comment.
Multi-line parameter lists are double-indented.
gerashegalov
force-pushed
the
gera/register-SHS-port-conf
branch
from
04fa7fd to
03e0e27
Compare
|
Thanks @vanzin for review. Please take a look again |
|
ok to test |
|
Test build #85705 has finished for PR 20098 at commit
|
|
Merging to master / 2.3. |
## What changes were proposed in this pull request? Register spark.history.ui.port as a known spark conf to be used in substitution expressions even if it's not set explicitly. ## How was this patch tested? Added unit test to demonstrate the issue Author: Gera Shegalov <[email protected]> Author: Gera Shegalov <[email protected]> Closes #20098 from gerashegalov/gera/register-SHS-port-conf. (cherry picked from commit ea95683) Signed-off-by: Marcelo Vanzin <[email protected]>
|
Thank you for review and commit @vanzin ! |
4 participants
What changes were proposed in this pull request?
Register spark.history.ui.port as a known spark conf to be used in substitution expressions even if it's not set explicitly.
How was this patch tested?
Added unit test to demonstrate the issue