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.

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

SNOW-917458 Snowflake JDBC drivers fails on JDK 21 with Arrow #1529

Merged
merged 1 commit into from
Nov 22, 2023

Conversation

sfc-gh-wfateem
Copy link
Collaborator

@sfc-gh-wfateem sfc-gh-wfateem commented Oct 12, 2023

Cherry picking fix for GH-35053 into Arrow v10.0.

Overview

SNOW-917458

Fixes #1512

@sfc-gh-wfateem
Copy link
Collaborator Author

Reviewing failures. I also need to amend the pom.xml for the FIPS version of the JDBC driver.

@sfc-gh-wfateem sfc-gh-wfateem marked this pull request as draft October 13, 2023 14:40
@sfc-gh-wfateem sfc-gh-wfateem self-assigned this Oct 13, 2023
@sfc-gh-wfateem sfc-gh-wfateem marked this pull request as ready for review October 13, 2023 18:01
@sfc-gh-wfateem
Copy link
Collaborator Author

Ready for review.

@sfc-gh-igarish
Copy link
Collaborator

@sfc-gh-wfateem did you build the ARROW libs or downloaded from Apache site? We couldn't use directly from Apache site as it spill some logs when Arrow libs initialized and we had a regression in the past. So we have arrow repo in monorepo and build over there. Please check.

@sfc-gh-igarish
Copy link
Collaborator

@sfc-gh-skumbham please review it as you worked on revert arrow libs last month.

@sfc-gh-wfateem
Copy link
Collaborator Author

@sfc-gh-igarish I built the Arrow dependencies, but I just used the OSS GitHub repository. I'm not actually familiar with the issue you're referring to so I would need to sync up with someone about that. In any case, this looks like it will break support for JDK 8. In order to make this work I had to upgrade the Netty library version which seemed to introduce a crashing issue on a JDK 8 runtime environment:
netty/netty/#12119

We'll either need to hold off until there's a fix, or this would have to be a breaking change release. There has been a conversation about JDK 8 support since that's reached the end of public updates.

@sfc-gh-wfateem sfc-gh-wfateem marked this pull request as draft October 13, 2023 19:05
@sfc-gh-wfateem
Copy link
Collaborator Author

It looks like it's a problem on just some JDK 8 versions, but also in my case removing the following JVM argument addressed the issue:

 -XX:StartFlightRecording=name=background,maxsize=200m,filename=jfr_data.jfr

So it's still a breaking change. I think it's important that we support JDK 21, but we would have to make it abundantly clear that a new release may break JDK 8 runtime environments. Unless we want to wait for a fix.

I'm looking to see if there's a way around this somehow.

@sonarqubemergegate
Copy link

SonarQube Quality Gate

Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@wendigo
Copy link

wendigo commented Oct 24, 2023

Any news?

@Komdosh
Copy link
Contributor

Komdosh commented Nov 2, 2023

Any updates?

@sfc-gh-wfateem
Copy link
Collaborator Author

There was a discussion around some security issues with other libraries and it was decided to upgrade Jackson separately in PR #1545 (SNOW-924672). This PR will just include the Arrow library upgrade.
@wendigo @Komdosh sorry for the delay, I was out for a few days. I'll get around to this by next week at the latest.

@wendigo
Copy link

wendigo commented Nov 7, 2023

@sfc-gh-wfateem any progress? :) 21.0.1 is out

@tpoll
Copy link

tpoll commented Nov 11, 2023

Any updates? Is there anything to community can do to help this progress?

@sajjoseph
Copy link

We are also impacted by this issue. Any quick updates?

@Komdosh
Copy link
Contributor

Komdosh commented Nov 15, 2023

@sfc-gh-wfateem
Any new updates?

@sfc-gh-wfateem
Copy link
Collaborator Author

@Komdosh @tpoll sorry for the delay. I've been out for a while and I had an unfortunate mishap with my laptop, so I only got a replacement recently.
The challenge is that we maintain our own Arrow library dependency because of some historical reason around logging, which I'm not fully aware of. That's probably something we want to explore to find out if that problem is still applicable or not with the latest Arrow library version, because if not then I don't see why we have to continue maintaining Arrow separately. That would also block the other initiative to release a non-shaded version of the JDBC driver.

The original PR I created included the open source Apache Arrow dependency while I should be building that using our version instead. I don't have access to that particular repository which is why I haven't been able to complete this PR as of yet. I'm working on either getting access to that repository or have someone else with access assist with building the required dependency.

I appreciate your patience here.

@sajjoseph
Copy link

Thanks @sfc-gh-wfateem.
I was able to manually build a version of snowflake-jdbc driver with your changes in and it is working in JDK 21.

Cherry picked Arrow fix GH-35053 into Arrow v10.0.1
@wendigo
Copy link

wendigo commented Nov 22, 2023

@sfc-gh-wfateem do you plan to merge and release this change anytime soon?

Copy link

SonarQube Quality Gate

Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Collaborator

@sfc-gh-ext-simba-jl sfc-gh-ext-simba-jl left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@sfc-gh-igarish sfc-gh-igarish left a comment

Choose a reason for hiding this comment

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

LGTM

@sfc-gh-wfateem sfc-gh-wfateem merged commit d047760 into master Nov 22, 2023
28 checks passed
@sfc-gh-wfateem sfc-gh-wfateem deleted the SNOW-917458-poc branch November 22, 2023 20:00
@github-actions github-actions bot locked and limited conversation to collaborators Nov 22, 2023
@sfc-gh-wfateem
Copy link
Collaborator Author

@wendigo thanks for your patience.
The fix is now merged and that should be included in the next release. I think that should roughly be the first half of December.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SNOW-917458: Snowflake JDBC drivers fails on JDK 21 with Arrow serialization
7 participants