[SPARK-32124][CORE][SHS] Fix taskEndReasonFromJson to handle event logs from old Spark versions #28941

warrenzhu25 · 2020-06-28T20:33:39Z

What changes were proposed in this pull request?

Fix bug of exception when parse event log of fetch failed task end reason without Map Index

Why are the changes needed?

When Spark history server read event log produced by older version of spark 2.4 (which don't have Map Index field), parsing of TaskEndReason will fail. This will cause TaskEnd event being ignored.

Does this PR introduce any user-facing change?

No

How was this patch tested?

JsonProtocolSuite.test("FetchFailed Map Index backwards compatibility")

…enerate the shuffle files

viirya · 2020-06-28T20:59:36Z

ok to test

SparkQA · 2020-06-28T21:04:33Z

Test build #124609 has finished for PR 28941 at commit b66fa2b.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2020-06-28T21:48:38Z

Thank you for your first contribution, @warrenzhu25 .

Could you file a new JIRA issue for this?
Also, it would be great if you can use a proper title for your PR content.

core/src/main/scala/org/apache/spark/util/JsonProtocol.scala

dongjoon-hyun · 2020-06-28T21:53:22Z

cc @xuanyuanking , @cloud-fan , @gatorsmile

viirya · 2020-06-28T23:11:50Z

retest this please

core/src/test/scala/org/apache/spark/util/JsonProtocolSuite.scala

SparkQA · 2020-06-29T02:28:19Z

Test build #124612 has finished for PR 28941 at commit b66fa2b.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-06-29T02:42:27Z

Test build #124613 has finished for PR 28941 at commit 910e285.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun

+1, LGTM. Thank you so much, @warrenzhu25 and @viirya .
Merged to master/3.0.

…gs from old Spark versions ### What changes were proposed in this pull request? Fix bug of exception when parse event log of fetch failed task end reason without `Map Index` ### Why are the changes needed? When Spark history server read event log produced by older version of spark 2.4 (which don't have `Map Index` field), parsing of TaskEndReason will fail. This will cause TaskEnd event being ignored. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? JsonProtocolSuite.test("FetchFailed Map Index backwards compatibility") Closes #28941 from warrenzhu25/shs-task. Authored-by: Warren Zhu <zhonzh@microsoft.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit 197ac3b) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

dongjoon-hyun · 2020-06-29T04:09:11Z

SPARK-32124 is assigned to you, @warrenzhu25 .

HeartSaVioR · 2020-06-29T04:48:39Z

Late LGTM. Thanks for taking care of!

xuanyuanking

Thanks for the fixing! Late LGTM.

Sorry for the late. Because of the fix has been merged, I think it's unnecessary to submit a follow-up PR for the small comment. If this file has change next time, maybe we can consider addressing this.

xuanyuanking · 2020-06-30T07:49:32Z

core/src/main/scala/org/apache/spark/util/JsonProtocol.scala

@@ -1078,7 +1078,10 @@ private[spark] object JsonProtocol {
        val blockManagerAddress = blockManagerIdFromJson(json \ "Block Manager Address")
        val shuffleId = (json \ "Shuffle ID").extract[Int]
        val mapId = (json \ "Map ID").extract[Long]
-        val mapIndex = (json \ "Map Index").extract[Int]
+        val mapIndex = (json \ "Map Index") match {
+          case JNothing => 0


nit: 0 is a legal value for the map index here. Since it's for backward compatibility, an illegal value here should make more sense. Also, it's better to comment here for emphasizing it's for backward compatibility, otherwise the fetch failed event will be dropped when SHS loading old version logs.

Yeah that's valid concern, and caller may need to be prepared to handle the invalid value. Why not we fix it for FOLLOW-UP PR? We have been doing it even for nits, and this doesn't even look to be a nit.

Make sense. It deserves a follow-up, let me submit one soon.

…fill the map index when the event logs from the old Spark version ### What changes were proposed in this pull request? Use the invalid value Int.MinValue to fill the map index when the event logs from the old Spark version. ### Why are the changes needed? Follow up PR for #28941. ### Does this PR introduce _any_ user-facing change? When we use the Spark version 3.0 history server reading the event log written by the old Spark version, we use the invalid value -2 to fill the map index. ### How was this patch tested? Existing UT. Closes #28965 from xuanyuanking/follow-up. Authored-by: Yuanjian Li <xyliyuanjian@gmail.com> Signed-off-by: Jungtaek Lim (HeartSaVioR) <kabhwan.opensource@gmail.com>

…fill the map index when the event logs from the old Spark version ### What changes were proposed in this pull request? Use the invalid value Int.MinValue to fill the map index when the event logs from the old Spark version. ### Why are the changes needed? Follow up PR for #28941. ### Does this PR introduce _any_ user-facing change? When we use the Spark version 3.0 history server reading the event log written by the old Spark version, we use the invalid value -2 to fill the map index. ### How was this patch tested? Existing UT. Closes #28965 from xuanyuanking/follow-up. Authored-by: Yuanjian Li <xyliyuanjian@gmail.com> Signed-off-by: Jungtaek Lim (HeartSaVioR) <kabhwan.opensource@gmail.com> (cherry picked from commit 3659611) Signed-off-by: Jungtaek Lim (HeartSaVioR) <kabhwan.opensource@gmail.com>

Warren Zhu added 2 commits June 28, 2020 13:13

[SPARK-25341][CORE] Support rolling back a shuffle map stage and re-g…

3a151a2

…enerate the shuffle files

Add UT for task FetchFailed reason with map index

b66fa2b

probot-autolabeler bot added the CORE label Jun 28, 2020

dongjoon-hyun reviewed Jun 28, 2020

View reviewed changes

core/src/main/scala/org/apache/spark/util/JsonProtocol.scala Show resolved Hide resolved

warrenzhu25 changed the title ~~[SPARK-25341][CORE][FOLLOWUP] Support rolling back a shuffle map stage and re-generate the shuffle files~~ [SHS] Failed to parse FetchFailed TaskEndReason from event log produce by Spark 2.4 Jun 28, 2020

warrenzhu25 changed the title ~~[SHS] Failed to parse FetchFailed TaskEndReason from event log produce by Spark 2.4~~ [SPARK-32124][SHS] Failed to parse FetchFailed TaskEndReason from event log produce by Spark 2.4 Jun 28, 2020

viirya reviewed Jun 28, 2020

View reviewed changes

core/src/test/scala/org/apache/spark/util/JsonProtocolSuite.scala Outdated Show resolved Hide resolved

Add Jira issue in UT

910e285

dongjoon-hyun changed the title ~~[SPARK-32124][SHS] Failed to parse FetchFailed TaskEndReason from event log produce by Spark 2.4~~ [SPARK-32124][CORE][SHS] Failed to parse FetchFailed TaskEndReason from event log produce by Spark 2.4 Jun 29, 2020

dongjoon-hyun changed the title ~~[SPARK-32124][CORE][SHS] Failed to parse FetchFailed TaskEndReason from event log produce by Spark 2.4~~ [SPARK-32124][CORE][SHS] Failed to parse FetchFailed TaskEndReason from event log produce by old Spark versions Jun 29, 2020

dongjoon-hyun approved these changes Jun 29, 2020

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-32124][CORE][SHS] Failed to parse FetchFailed TaskEndReason from event log produce by old Spark versions~~ [SPARK-32124][CORE][SHS] Fix taskEndReasonFromJson to handle event logs from old Spark versions Jun 29, 2020

dongjoon-hyun closed this in 197ac3b Jun 29, 2020

xuanyuanking reviewed Jun 30, 2020

View reviewed changes

xuanyuanking mentioned this pull request Jul 1, 2020

[SPARK-32124][CORE][FOLLOW-UP] Use the invalid value Int.MinValue to fill the map index when the event logs from the old Spark version #28965

Closed

warrenzhu25 deleted the shs-task branch June 11, 2022 17:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-32124][CORE][SHS] Fix taskEndReasonFromJson to handle event logs from old Spark versions #28941

[SPARK-32124][CORE][SHS] Fix taskEndReasonFromJson to handle event logs from old Spark versions #28941

warrenzhu25 commented Jun 28, 2020

viirya commented Jun 28, 2020

SparkQA commented Jun 28, 2020

dongjoon-hyun commented Jun 28, 2020

dongjoon-hyun commented Jun 28, 2020

viirya commented Jun 28, 2020

SparkQA commented Jun 29, 2020

SparkQA commented Jun 29, 2020

dongjoon-hyun left a comment

dongjoon-hyun commented Jun 29, 2020

HeartSaVioR commented Jun 29, 2020

xuanyuanking left a comment

xuanyuanking Jun 30, 2020

HeartSaVioR Jul 1, 2020 •

edited

Loading

xuanyuanking Jul 1, 2020

[SPARK-32124][CORE][SHS] Fix taskEndReasonFromJson to handle event logs from old Spark versions #28941

[SPARK-32124][CORE][SHS] Fix taskEndReasonFromJson to handle event logs from old Spark versions #28941

Conversation

warrenzhu25 commented Jun 28, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

viirya commented Jun 28, 2020

SparkQA commented Jun 28, 2020

dongjoon-hyun commented Jun 28, 2020

dongjoon-hyun commented Jun 28, 2020

viirya commented Jun 28, 2020

SparkQA commented Jun 29, 2020

SparkQA commented Jun 29, 2020

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Jun 29, 2020

HeartSaVioR commented Jun 29, 2020

xuanyuanking left a comment

Choose a reason for hiding this comment

xuanyuanking Jun 30, 2020

Choose a reason for hiding this comment

HeartSaVioR Jul 1, 2020 • edited Loading

Choose a reason for hiding this comment

xuanyuanking Jul 1, 2020

Choose a reason for hiding this comment

HeartSaVioR Jul 1, 2020 •

edited

Loading