-
Notifications
You must be signed in to change notification settings - Fork 28.6k
[SPARK-24601][SPARK-27051][BACKPORT][CORE] Update to Jackson 2.9.8 #24418
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@@ -224,6 +224,10 @@ | |||
<groupId>org.scala-lang</groupId> | |||
<artifactId>scala-library</artifactId> | |||
</dependency> | |||
<dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We used to get this transitively from Jackson; need to make it explicit now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, this one is nasty, we've had soo many conflict on this one..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This much isn't a change to the final dependency graph, at least. It always need/depended on scala-reflect
@@ -36,7 +36,7 @@ import org.apache.spark.util.Utils | |||
* (2) the Spark version of the client / server | |||
* (3) an optional message | |||
*/ | |||
@JsonInclude(Include.NON_NULL) | |||
@JsonInclude(Include.NON_ABSENT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the core of the change to maintain behavior, as NON_NULL changed behavior in 2.7 and 2.8
Test build #104758 has finished for PR 24418 at commit
|
cc @dbtsai |
LGTM |
LGTM. |
Test build #104760 has finished for PR 24418 at commit
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, LGTM. Merged to branch-2.4.
This is a collective backport (except 3.0-only ExecutorMetricsJsonDeserializer
class).
Thank you, @srowen , @dbtsai , @felixcheung , @arunmahadevan , @Fokko !
## What changes were proposed in this pull request? This backports: ab1650d 7857c6d which collectively updates Jackson to 2.9.8. ## How was this patch tested? Existing tests. Closes #24418 from srowen/SPARK-24601.2. Authored-by: Sean Owen <sean.owen@databricks.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
I support this, and wanted to more fully explain the logic, as it occurs to me not all of it was visible in the conversation here:
Weighing that against the relatively small behavior change I think it's the right thing for 2.4.x. This won't go into 2.4.2 unless there is another RC. If not, it'll be in 2.4.3. |
## What changes were proposed in this pull request? This backports: apache@ab1650d apache@7857c6d which collectively updates Jackson to 2.9.8. ## How was this patch tested? Existing tests. Closes apache#24418 from srowen/SPARK-24601.2. Authored-by: Sean Owen <sean.owen@databricks.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
## What changes were proposed in this pull request? This backports: apache@ab1650d apache@7857c6d which collectively updates Jackson to 2.9.8. ## How was this patch tested? Existing tests. Closes apache#24418 from srowen/SPARK-24601.2. Authored-by: Sean Owen <sean.owen@databricks.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
## What changes were proposed in this pull request? This backports: apache@ab1650d apache@7857c6d which collectively updates Jackson to 2.9.8. ## How was this patch tested? Existing tests. Closes apache#24418 from srowen/SPARK-24601.2. Authored-by: Sean Owen <sean.owen@databricks.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
What changes were proposed in this pull request?
This backports:
ab1650d
7857c6d
which collectively updates Jackson to 2.9.8.
How was this patch tested?
Existing tests.