[SPARK-22914][DEPLOY] Register history.ui.port #20098
Conversation
|
CC @vanzin |
| @@ -836,6 +833,20 @@ object ApplicationMaster extends Logging { | |||
|
|
|||
| } | |||
|
|
|||
| object ApplicationMasterUtil { | |||
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.
| import org.scalatest.Matchers | ||
|
|
||
| class ApplicationMasterUtilSuite extends SparkFunSuite with Matchers with Logging | ||
| with ResetSystemProperties { |
There was a problem hiding this comment.
ResetSystemProperties not needed since you're not changing system properties.
| import org.apache.spark.util.ResetSystemProperties | ||
| import org.scalatest.Matchers | ||
|
|
||
| class ApplicationMasterUtilSuite extends SparkFunSuite with Matchers with Logging |
There was a problem hiding this comment.
Logging not needed, you're not logging anything.
| val shsAddr = ApplicationMasterUtil | ||
| .getHistoryServerAddress(sparkConf, yarnConf, appId, attemptId) | ||
|
|
||
| shsAddr shouldEqual "http://rm.host.com:18080/history/application_123_1/application_123_1_1" |
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.
|
|
||
| package org.apache.spark.deploy.yarn | ||
|
|
||
| import org.apache.hadoop.yarn.conf.YarnConfiguration |
There was a problem hiding this comment.
These imports will fail style checks. Make sure to run them locally.
| @@ -836,6 +833,20 @@ object ApplicationMaster extends Logging { | |||
|
|
|||
| } | |||
|
|
|||
| object ApplicationMasterUtil { | |||
| def getHistoryServerAddress( | |||
| sparkConf: SparkConf, | |||
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 <gera@apache.org> Author: Gera Shegalov <gshegalov@salesforce.com> Closes #20098 from gerashegalov/gera/register-SHS-port-conf. (cherry picked from commit ea95683) Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>
|
Thank you for review and commit @vanzin ! |
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