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

feat: Native image support for R2DBC driver #715

Merged
merged 45 commits into from
Aug 21, 2023
Merged

Conversation

jainsahab
Copy link
Contributor

No description provided.

Comment on lines 19 to 32
- java: '11'
version: '22.3.2'
components: 'native-image'
name: 'Java 11 | GraalVM 22.3.2'
- java: '17'
version: '22.3.2'
components: 'native-image'
name: 'Java 17 | GraalVM 22.3.2'
- java: '17.0.8'
distribution: 'graalvm'
name: 'GraalVM for Java 17 | 17.0.8'
- java: '20.0.2'
distribution: 'graalvm'
name: 'GraalVM for Java 20 | 20.0.2'
Copy link
Member

Choose a reason for hiding this comment

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

@mpeddada1 @burkedavison Is this the right set of versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just for reference, first two are 22.3.2 version with Java 11 and 17 similar to what we have internally for client libraries.

and the last two are for the latest GraalVM release (23.x) for JDK 17 and JDK 20. No support for JDK 11.

Copy link
Member

Choose a reason for hiding this comment

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

We aren't yet supporting the last two, but if the tests are passing it doesn't hurt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

update: removed the latest GraalVM release (23.x) for JDK 17 and JDK 20 in 62b1506.

.mvn/wrapper/maven-wrapper.properties Outdated Show resolved Hide resolved
Comment on lines 1 to 5
[
{
"name":"com.google.rpc.BadRequest",
"methods":[{"name":"getFieldViolations","parameterTypes":["int"] }, {"name":"getFieldViolationsCount","parameterTypes":[] }, {"name":"getFieldViolationsList","parameterTypes":[] }]
},

Choose a reason for hiding this comment

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

@jainsahab Do we happen to have a stacktrace of the error message we run into if we don't include these configurations? Mostly curious what class or dependency needs it. If it is come from a client library or it's dependency (like gax) then we may be adding the configuration too late in the release cycle.

Copy link
Contributor Author

@jainsahab jainsahab Aug 7, 2023

Choose a reason for hiding this comment

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

sure @mpeddada1 , here it is.

image

i believe this is coming from gax-grpc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One thing i would like to point out @mpeddada1 that this is just happening for the latest GraalVM releases (23.x) for JDK 17 and JDK 20

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's been deleted now as part of 62b1506

pom.xml Outdated
<buildArg>--no-fallback</buildArg>
<buildArg>--no-server</buildArg>
<!-- Until https://github.com/googleapis/sdk-platform-java/pull/1892 is merged -->
<buildArg>-H:+RunReachabilityHandlersConcurrently</buildArg>

Choose a reason for hiding this comment

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

The argument in native-image.properties should automatically get picked up by the plugin so we may not need to add it in here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i tried removing this option from pom.xml on GraalVM for java 17, and this is the error i am getting consistently.

Screenshot 2023-08-07 at 1 43 50 PM

But when i explicitly pass this option in pom.xml, I am able to build it successfully, we can also see that this argument is being passed to native-image command line options.

Screenshot 2023-08-07 at 1 44 11 PM

Copy link
Member

Choose a reason for hiding this comment

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

Until we can turn off this flag in early September, the configuration here might conflict with our other client libraries.

We shouldn't be spending too much time trying to get support for currently-unsupported versions of GraalVM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, removed the configuration for GraalVM for java 17 and 20.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 9, 2023

Kudos, SonarCloud Quality Gate passed!    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
0.0% 0.0% Duplication

warning The version of Java (11.0.20) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@meltsufin meltsufin merged commit 44463cf into main Aug 21, 2023
@meltsufin meltsufin deleted the driver_native_support branch August 21, 2023 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants