Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[CALCITE-5374] Upgrade jackson from 2.10.0 to 2.14.1 #195

Merged
merged 1 commit into from
Nov 28, 2022
Merged

[CALCITE-5374] Upgrade jackson from 2.10.0 to 2.14.1 #195

merged 1 commit into from
Nov 28, 2022

Conversation

snuyanzin
Copy link
Contributor

The PR upgrading jackson to 2.14.0

@F21
Copy link
Member

F21 commented Nov 9, 2022 •
edited
Loading

@snuyanzin Can you please take a look at the failing tests for JDK8 (Avatica main)?

@snuyanzin
Copy link
Contributor Author

yes, sure will try find the reason

@snuyanzin
Copy link
Contributor Author

snuyanzin commented Nov 12, 2022 •
edited
Loading

It seems the reason is that in 2.14.0 com.fasterxml.jackson.databind.node.ObjectNode#with became deprecated FasterXML/jackson-databind#3568
So first need to update it in Calcite and replace deprecated methods and then could be updated for Avatica.

PR for Calcite apache/calcite#2966

@julianhyde
Copy link
Contributor

Now I've merged the fix into Calcite, the Avatica JDK 8 test should pass. Once it passes let's merge this PR.

@snuyanzin
Copy link
Contributor Author

Thanks for merging.
If you don't mind i would update it to 2.14.1 to include fix for ObjectMapper default heap consumption increased significantly from 2.13.x to 2.14.0

I will try to upgrade both Calcite and Avatica today

@snuyanzin snuyanzin changed the title [CALCITE-5374] Upgrade jackson to 2.14.0 [CALCITE-5374] Upgrade jackson from 2.10.0 to 2.14.1 Nov 28, 2022
F21
F21 approved these changes Nov 28, 2022
View reviewed changes
Copy link
Member

@F21 F21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@snuyanzin snuyanzin closed this in 42f53ff Nov 28, 2022
@snuyanzin snuyanzin merged commit 42f53ff into apache:main Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@F21 F21 F21 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@snuyanzin @F21 @julianhyde