[DRAFT][SPARK-23607][CORE] Use HDFS extended attributes to store application summary information in SHS#40949
[DRAFT][SPARK-23607][CORE] Use HDFS extended attributes to store application summary information in SHS#40949thejdeep wants to merge 1 commit intoapache:masterfrom
Conversation
This PR seeks to improve the performance of serving the application list in History Server by storing the required information of the application as part of HDFS extended attributes instead of parsing the log file each time. ### Why are the changes needed? Improves the performance of the History Server listing page ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Unit tests to be added
mridulm
left a comment
mridulm
left a comment
There was a problem hiding this comment.
Took one pass through the changes.
We need to add unit tests to validate these changes are working as expected.
| case _: IOException => | ||
| logWarning(s"Failed to set extended attribute ${attrName}") | ||
| case _: UnsupportedOperationException => | ||
| logWarning("setXAttr not supported by filesystem") |
| .createOptional | ||
|
|
||
| val EVENT_LOG_XATTR_ENABLED = ConfigBuilder("spark.history.fs.eventLog.xattr.enabled") | ||
| .version("3.3.0") |
| None, reader.completed)) | ||
| count = count + 1 | ||
| reader.fileSizeForLastIndex > 0 | ||
| handleNewLogFile(reader, newLastScanTime) |
| var xAttrStatus = LogXAttrStatus.XATTR_DISABLED | ||
| if(HISTORY_LOG_USE_XATTR) { | ||
| val isXAttrEnabled = getXAttr(reader.rootPath, EventLoggingListener.USER_XATTR_ENABLED) | ||
| if (isXAttrEnabled.isDefined && new String(isXAttrEnabled.get) == "true") { |
There was a problem hiding this comment.
Encode/decode bytes explicitly using UTF-8, and not platform default.
nit: Also, use something like
xattrEnabled.exists(data => new String(data, StandardCharsets.UTF_8) == "true") instead of the current condition
| private def handleNewLogFile(reader: EventLogFileReader, scanTime: Long): Boolean = { | ||
| var xAttrStatus = LogXAttrStatus.XATTR_DISABLED | ||
| if(HISTORY_LOG_USE_XATTR) { | ||
| val isXAttrEnabled = getXAttr(reader.rootPath, EventLoggingListener.USER_XATTR_ENABLED) |
There was a problem hiding this comment.
nit: isXAttrEnabled -> xattrEnabled
| case _: NoSuchElementException => | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is mostly duplicated in doMergeApplicationListingInternal - and there is some changes between both - please update this method and ensure doMergeApplicationListingInternal delegates to this method as well.
| try { | ||
| val valueMap = fs.getXAttrs(path, nameList.asJava) | ||
| if(valueMap != null) { | ||
| Some(valueMap.asScala.toMap.map(value => value._1 -> new String(value._2))) |
There was a problem hiding this comment.
As mentioned earlier, use utf-8 when converting bytes to String
| private def getXAttr(path: Path, name: String): Option[String] = { | ||
| val valueMap = getXAttrs(path, List(name)) | ||
| if (!valueMap.isEmpty) { | ||
| Some(valueMap.get(name)) |
There was a problem hiding this comment.
nit: Option instead of Some
| var name: String = null | ||
|
|
||
| def toView(attempts: List[AttemptInfoWrapper]): ApplicationInfoWrapper = { | ||
| val apiInfo = ApplicationInfo(id.get, name, None, None, None, None, Nil) |
There was a problem hiding this comment.
Why are we assuming id wont be None ?
If that is not a valid assumption, replace with id.orNull
| viewAcls, | ||
| adminAclsGroups, | ||
| viewAclsGroups) | ||
| } |
There was a problem hiding this comment.
What changed in MutableAttemptInfo ?
Also, why did we pull it out ?
|
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
2 participants
What changes were proposed in this pull request?
Previously, #34829 was raised which was auto closed due to staleness. This is a rework of that by addressing comments raised.
This PR seeks to improve the performance of serving the application list in History Server by storing the required information of the application as part of HDFS extended attributes instead of parsing the log file each time.
Why are the changes needed?
Improves the performance of the History Server listing page
Below is the comparison of the time taken to complete
mergeApplicationListingcall in FsHistoryProvider that is called for every log file that is updated in the event log directory :As we can see in the comparison above, irrespective of the event log size, the time to build the application listing for the app remains the same. This is hugely beneficial in clusters that have very large event log sizes.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Will add unit tests