[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

xuanyuanking · 2020-07-01T08:21:53Z

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.

xuanyuanking · 2020-07-01T08:22:37Z

cc @warrenzhu25 @HeartSaVioR @dongjoon-hyun

SparkQA · 2020-07-01T11:37:21Z

Test build #124760 has finished for PR 28965 at commit 9b9e4ee.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

warrenzhu25 · 2020-07-01T16:34:30Z

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

+            // Note, we use the invalid value -2 here to fill the map index for backward
+            // compatibility. Otherwise, the fetch failed event will be dropped when the history
+            // server loads the event log written by the Spark version before 3.0.
+            -2


Any special reasons using -2 instead of -1? BTW, I'm unsure whether this filed will be displayed in UI, if so, we better show Unknown for invalid value in UI.

Yeah once we provide it with magic number to mark INVALID/UNKNOWN, there should be relevant code change to handle INVALID/UNKNOWN value. Looks like the code change doesn't address it, which makes me unsure how it will be represented on the UI.

@HeartSaVioR I think it is represented as error string in FetchFailed.toErrorString. So we just need to make sure the default unknown value is consistent in the code here and FetchFailed.toErrorString. Personally I prefer Int.MinValue since -1 can't be used here.

Thanks Gengliang for confirming!

Any special reasons using -2 instead of -1?

mapIndex = -1 has been used in MetadataFetchFailedException.

Yep, I think Int.MinValue is a better choice.

Thanks for the info! If the value is only leveraged via FetchFailed.toErrorString, what we may want to do (and proposed in previous comment) is projecting the magic number to the specific string so that it can be determined easily. +1 to Int.MinValue.

And I'm seeing it's done. Thanks for the quick address!

Yep, already addressed in 9dfa5c0

SparkQA · 2020-07-02T08:35:33Z

Test build #124884 has finished for PR 28965 at commit 9dfa5c0.

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

dongjoon-hyun · 2020-07-02T15:22:45Z

Hi, @HeartSaVioR . Could you finalize this as an Apache Spark committer? :)

gengliangwang · 2020-07-02T17:05:09Z

@HeartSaVioR Congratulations!

HeartSaVioR

LGTM pending Jenkins

HeartSaVioR · 2020-07-02T22:51:53Z

retest this, please

HeartSaVioR · 2020-07-02T22:54:29Z

Btw thanks @dongjoon-hyun and @gengliangwang ! I thought it's probably better to take committer's work after official announcement, but this looks to be simple and good example to try out before dealing with larger PR. Thanks again for the guidance!

dongjoon-hyun · 2020-07-02T22:58:01Z

Ya. That's also reasonable. Usually, the annoucement gets some delays because the invitations go out to multiple persons at the same season. You don't need to be blocked. ;) You've been waiting so long. Please do what you want in your way.

HeartSaVioR · 2020-07-02T23:19:31Z

Jenkins doesn't seem to pick this up by request ;( Mentioned the behavior in the dev@ list.

gengliangwang · 2020-07-03T00:21:02Z

retest this please

xuanyuanking · 2020-07-03T04:05:53Z

Wow, Congratulations @HeartSaVioR! Well deserved!

SparkQA · 2020-07-05T20:06:14Z

Test build #124936 has finished for PR 28965 at commit 9dfa5c0.

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

dongjoon-hyun · 2020-07-05T20:07:52Z

Retest this please.

SparkQA · 2020-07-05T20:17:04Z

Test build #124960 has finished for PR 28965 at commit 9dfa5c0.

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

HeartSaVioR · 2020-07-05T22:22:16Z

retest this, please

SparkQA · 2020-07-06T04:34:00Z

Test build #124972 has finished for PR 28965 at commit 9dfa5c0.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HeartSaVioR · 2020-07-06T09:26:53Z

retest this, please

SparkQA · 2020-07-06T13:51:16Z

Test build #125058 has finished for PR 28965 at commit 9dfa5c0.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HeartSaVioR · 2020-07-06T14:09:17Z

retest this, please

SparkQA · 2020-07-06T17:49:22Z

Test build #125089 has finished for PR 28965 at commit 9dfa5c0.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HeartSaVioR · 2020-07-06T20:04:42Z

retest this, please

SparkQA · 2020-07-07T00:07:18Z

Test build #125122 has finished for PR 28965 at commit 9dfa5c0.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HeartSaVioR · 2020-07-07T03:59:24Z

retest this, please

SparkQA · 2020-07-07T07:05:02Z

Test build #125171 has finished for PR 28965 at commit 9dfa5c0.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

xuanyuanking · 2020-07-07T07:36:46Z

retest this please

SparkQA · 2020-07-07T11:00:53Z

Test build #125190 has finished for PR 28965 at commit 9dfa5c0.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HeartSaVioR · 2020-07-07T12:00:59Z

retest this please

SparkQA · 2020-07-07T14:35:03Z

Test build #125216 has finished for PR 28965 at commit 9dfa5c0.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

HeartSaVioR · 2020-07-07T19:46:25Z

retest this please

SparkQA · 2020-07-08T00:33:00Z

Test build #125248 has finished for PR 28965 at commit 9dfa5c0.

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

…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 <[email protected]> Signed-off-by: Jungtaek Lim (HeartSaVioR) <[email protected]> (cherry picked from commit 3659611) Signed-off-by: Jungtaek Lim (HeartSaVioR) <[email protected]>

HeartSaVioR · 2020-07-08T00:37:39Z

Thanks! Merged to master and branch-3.0.

HyukjinKwon · 2020-07-08T00:47:44Z

The very first commit merged by @HeartSaVioR!

change to invalid value

9b9e4ee

probot-autolabeler bot added the CORE label Jul 1, 2020

warrenzhu25 reviewed Jul 1, 2020

View reviewed changes

address comments

9dfa5c0

HeartSaVioR approved these changes Jul 2, 2020

View reviewed changes

gengliangwang approved these changes Jul 3, 2020

View reviewed changes

HeartSaVioR closed this in 3659611 Jul 8, 2020

xuanyuanking deleted the follow-up branch July 16, 2020 05:45

[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

[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

Uh oh!

Conversation

xuanyuanking commented Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

xuanyuanking commented Jul 1, 2020

Uh oh!

SparkQA commented Jul 1, 2020

Uh oh!

warrenzhu25 Jul 1, 2020

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR Jul 1, 2020

Choose a reason for hiding this comment

Uh oh!

gengliangwang Jul 2, 2020

Choose a reason for hiding this comment

Uh oh!

xuanyuanking Jul 2, 2020

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR Jul 2, 2020

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR Jul 2, 2020

Choose a reason for hiding this comment

Uh oh!

xuanyuanking Jul 2, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 2, 2020

Uh oh!

dongjoon-hyun commented Jul 2, 2020

Uh oh!

gengliangwang commented Jul 2, 2020

Uh oh!

HeartSaVioR left a comment

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR commented Jul 2, 2020

Uh oh!

HeartSaVioR commented Jul 2, 2020

Uh oh!

dongjoon-hyun commented Jul 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HeartSaVioR commented Jul 2, 2020

Uh oh!

gengliangwang commented Jul 3, 2020

Uh oh!

xuanyuanking commented Jul 3, 2020

Uh oh!

SparkQA commented Jul 5, 2020

Uh oh!

dongjoon-hyun commented Jul 5, 2020

Uh oh!

SparkQA commented Jul 5, 2020

Uh oh!

HeartSaVioR commented Jul 5, 2020

Uh oh!

SparkQA commented Jul 6, 2020

Uh oh!

HeartSaVioR commented Jul 6, 2020

Uh oh!

SparkQA commented Jul 6, 2020

Uh oh!

HeartSaVioR commented Jul 6, 2020

Uh oh!

SparkQA commented Jul 6, 2020

Uh oh!

HeartSaVioR commented Jul 6, 2020

Uh oh!

SparkQA commented Jul 7, 2020

Uh oh!

xuanyuanking commented Jul 1, 2020 •

edited

Loading

dongjoon-hyun commented Jul 2, 2020 •

edited

Loading