-
Notifications
You must be signed in to change notification settings - Fork 28.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-26443][CORE] Use ConfigEntry for hardcoded configs for history category. #23384
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,10 +25,18 @@ private[spark] object History { | |
|
||
val DEFAULT_LOG_DIR = "file:/tmp/spark-events" | ||
|
||
val EVENT_LOG_DIR = ConfigBuilder("spark.history.fs.logDirectory") | ||
val HISTORY_LOG_DIR = ConfigBuilder("spark.history.fs.logDirectory") | ||
.stringConf | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not related to this PR, but There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fine with the name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep. |
||
.createWithDefault(DEFAULT_LOG_DIR) | ||
|
||
val SAFEMODE_CHECK_INTERVAL_S = ConfigBuilder("spark.history.fs.safemodeCheck.interval") | ||
.timeConf(TimeUnit.SECONDS) | ||
.createWithDefaultString("5s") | ||
|
||
val UPDATE_INTERVAL_S = ConfigBuilder("spark.history.fs.update.interval") | ||
.timeConf(TimeUnit.SECONDS) | ||
.createWithDefaultString("10s") | ||
|
||
val CLEANER_ENABLED = ConfigBuilder("spark.history.fs.cleaner.enabled") | ||
.booleanConf | ||
.createWithDefault(false) | ||
|
@@ -79,4 +87,40 @@ private[spark] object History { | |
|
||
val MAX_DRIVER_LOG_AGE_S = ConfigBuilder("spark.history.fs.driverlog.cleaner.maxAge") | ||
.fallbackConf(MAX_LOG_AGE_S) | ||
|
||
val UI_ACLS_ENABLE = ConfigBuilder("spark.history.ui.acls.enable") | ||
.booleanConf | ||
.createWithDefault(false) | ||
|
||
val UI_ADMIN_ACLS = ConfigBuilder("spark.history.ui.admin.acls") | ||
.stringConf | ||
.createWithDefault("") | ||
|
||
val UI_ADMIN_ACLS_GROUPS = ConfigBuilder("spark.history.ui.admin.acls.groups") | ||
.stringConf | ||
.createWithDefault("") | ||
|
||
val NUM_REPLAY_THREADS = ConfigBuilder("spark.history.fs.numReplayThreads") | ||
.intConf | ||
.createWithDefaultFunction(() => Math.ceil(Runtime.getRuntime.availableProcessors() / 4f).toInt) | ||
|
||
val RETAINED_APPLICATIONS = ConfigBuilder("spark.history.retainedApplications") | ||
.intConf | ||
.createWithDefault(50) | ||
|
||
val PROVIDER = ConfigBuilder("spark.history.provider") | ||
.stringConf | ||
.createOptional | ||
|
||
val KERBEROS_ENABLED = ConfigBuilder("spark.history.kerberos.enabled") | ||
.booleanConf | ||
.createWithDefault(false) | ||
|
||
val KERBEROS_PRINCIPAL = ConfigBuilder("spark.history.kerberos.principal") | ||
.stringConf | ||
.createOptional | ||
|
||
val KERBEROS_KEYTAB = ConfigBuilder("spark.history.kerberos.keytab") | ||
.stringConf | ||
.createOptional | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,21 +22,22 @@ import java.nio.charset.StandardCharsets._ | |
import com.google.common.io.Files | ||
|
||
import org.apache.spark._ | ||
import org.apache.spark.internal.config.History._ | ||
import org.apache.spark.util.Utils | ||
|
||
class HistoryServerArgumentsSuite extends SparkFunSuite { | ||
|
||
private val logDir = new File("src/test/resources/spark-events") | ||
private val conf = new SparkConf() | ||
.set("spark.history.fs.logDirectory", logDir.getAbsolutePath) | ||
.set("spark.history.fs.updateInterval", "1") | ||
.set(HISTORY_LOG_DIR, logDir.getAbsolutePath) | ||
.set(UPDATE_INTERVAL_S, 1L) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
.set("spark.testing", "true") | ||
|
||
test("No Arguments Parsing") { | ||
val argStrings = Array.empty[String] | ||
val hsa = new HistoryServerArguments(conf, argStrings) | ||
assert(conf.get("spark.history.fs.logDirectory") === logDir.getAbsolutePath) | ||
assert(conf.get("spark.history.fs.updateInterval") === "1") | ||
assert(conf.get(HISTORY_LOG_DIR) === logDir.getAbsolutePath) | ||
assert(conf.get(UPDATE_INTERVAL_S) === 1L) | ||
assert(conf.get("spark.testing") === "true") | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to change this? Seems this pattern is quite used too...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like
spark.history.fs.updateInterval
is deprecated, but let me revert it for now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ur, it is good to remove the deprecated one as many as possible in Spark 3.0. When I searched this string during reviews, there were only two instances; here and deprecation checking test case. @mgaido91 , what do you mean by
quite used
? We already accepted this deprecated one and new one.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see only now that it was deprecated, sorry. Then, this is ok, sorry for the comment.
@dongjoon-hyun I meant that camel case notation for the last element is quite widespread also for other configs, so I didn't see the need for changing the notation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then should I update to include this change back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's okie. It's deprecated and we should get rid of it in the doc basically. I see no issue. Yea, I think it can be included back. LGTM without that change as well.