[SPARK-3562]Periodic cleanup event logs #4214
Conversation
| conf.getInt("spark.history.updateInterval", 10)) * 1000 | ||
| private val UPDATE_INTERVAL_MS = conf.getOption("spark.history.fs.update.interval.seconds") | ||
| .orElse(conf.getOption("spark.history.fs.updateInterval") | ||
| .map(warnUpdateInterval("spark.history.fs.updateInterval", _))) |
There was a problem hiding this comment.
Just for neatness, I'd have a single method that does both conf.getOption() and .map():
private def getDeprecatedConfig(conf: SparkConf, key: String): Option[String]
Then just do:
conf.getOption(newKey).orElse(getDeprecatedKey(old1)).orElse(getDeprecatedKey(old2))
|
LGTM aside from some very minor things. |
| @@ -163,9 +179,6 @@ private[history] class FsHistoryProvider(conf: SparkConf) extends ApplicationHis | |||
| * applications that haven't been updated since last time the logs were checked. | |||
| */ | |||
| private[history] def checkForLogs(): Unit = { | |||
| lastLogCheckTimeMs = getMonotonicTimeMs() | |||
| logDebug("Checking for logs. Time is now %d.".format(lastLogCheckTimeMs)) | |||
There was a problem hiding this comment.
why are you removing the logging?
There was a problem hiding this comment.
Good point. The tests don't expect anything to be happening in the background, instead they rely on being able to trigger these updates explicitly.
There was a problem hiding this comment.
checkForlogs thread is Scheduled by pool. lastLogCheckTimeMs is no use and remove it.
|
ok to test |
|
|
||
| private def getDeprecatedConfig(conf: SparkConf, key: String): Option[String] = { | ||
| conf.getOption(key).map(warnUpdateInterval(key, _)) | ||
| } |
There was a problem hiding this comment.
can you do all of these through SparkConf.deprecatedConfigs instead of doing it here? You may need to rebase to master to get those changes. (I filed SPARK-5931 for this purpose, and I hope to eventually move all the deprecation logic there).
|
@viper-kun Thanks for re-opening this, and my apologies for having neglected the older patch. I left a few comments inline, most of which are minor ones that have to do with naming and docs. Otherwise this patch looks fairly straightforward and I would like to get it merged soon. |
|
Test build #27791 has finished for PR 4214 at commit
|
|
Test build #27947 has finished for PR 4214 at commit
|
|
@andrewor14 thanks for your check. Pls retest it . I can not get test log. |
|
Test build #27974 has finished for PR 4214 at commit
|
|
Test build #27988 has finished for PR 4214 at commit
|
| conf.getInt("spark.history.updateInterval", 10)) * 1000 | ||
| private val UPDATE_INTERVAL_MS = conf.getOption("spark.history.fs.update.interval.seconds") | ||
| .orElse(conf.getOption(SparkConf.translateConfKey("spark.history.fs.updateInterval", true))) | ||
| .orElse(conf.getOption(SparkConf.translateConfKey("spark.history.updateInterval", true))) |
There was a problem hiding this comment.
We shouldn't call translateConfKey here, but I realize we need to do so if we want to warn the user. I will submit a separate patch to fix this behavior. In general I think the translateConfKey method should be private to SparkConf.
|
LGTM I'm merging this into master and I will file a new issue for the conf key issue I mentioned. Thanks for your patience @viper-kun! |
No description provided.