[SPARK-39636][CORE][UI] Fix multiple bugs in JsonProtocol, impacting off heap StorageLevels and Task/Executor ResourceRequests #37027
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JoshRosen
commented
Jun 30, 2022
| def taskResourceRequestFromJson(json: JValue): TaskResourceRequest = { | ||
| val rName = (json \ "Resource Name").extract[String] | ||
| val amount = (json \ "Amount").extract[Int] | ||
| val amount = (json \ "Amount").extract[Double] |
There was a problem hiding this comment.
amount is defined as a double at
JoshRosen
commented
Jun 30, 2022
| def executorResourceRequestFromJson(json: JValue): ExecutorResourceRequest = { | ||
| val rName = (json \ "Resource Name").extract[String] | ||
| val amount = (json \ "Amount").extract[Int] | ||
| val amount = (json \ "Amount").extract[Long] |
There was a problem hiding this comment.
amount is defined as a long at
JoshRosen
commented
Jun 30, 2022
| def storageLevelToJson(storageLevel: StorageLevel): JValue = { | ||
| ("Use Disk" -> storageLevel.useDisk) ~ | ||
| ("Use Memory" -> storageLevel.useMemory) ~ | ||
| ("Use Off Heap" -> storageLevel.useOffHeap) ~ |
There was a problem hiding this comment.
- The
useOffHeapfield was added in Spark 1.0 in PR SPARK-1305: Support persisting RDD's directly to Tachyon #158 and was persisted to JsonProtocol as a "Use Tachyon" field (since that was the original meaning of that field). - It was subsequently renamed to "Use ExternalBlockStore" in [SPARK-6479][Block Manager]Create off-heap block storage API #5430.
- It was later removed in [SPARK-12667] Remove block manager's internal "external block store" API #10752 as part of removal of the legacy external block store API.
- In [SPARK-13992] Add support for off-heap caching #11805 for Spark 2.x I repurposed the then-orphaned
OFF_HEAPstorage level for use with off-heap caching, but overlooked the fact that that theuseOffHeapfield was no longer being recorded inJsonProtocol
mridulm
approved these changes
Jun 30, 2022
|
Thanks for the reviews. I'm going to merge this to the master branch (Spark 3.4.0). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
This PR fixes three longstanding bugs in Spark's
JsonProtocol:TaskResourceRequestloses precision foramount< 0.5. Theamountis a floating point number which is either between 0 and 0.5 or is a positive integer, but the JSON read path assumes it is an integer.ExecutorResourceRequestinteger overflows for values larger than Int.MaxValue because the write path writes longs but the read path assumes integers.useOffHeapfield isn't included in the JSON, so this StorageLevel cannot be round-tripped through JSON. This could cause the History Server to display inaccurate "off heap memory used" stats on the executors page.I discovered these bugs while working on #36885.
Why are the changes needed?
JsonProtocol should be able to roundtrip events through JSON without loss of information.
Does this PR introduce any user-facing change?
Yes: it fixes bugs that impact information shown in the History Server Web UI. The new StorageLevel JSON field will be visible to tools which process raw event log JSON.
How was this patch tested?
Updated existing unit tests to cover the changed logic.