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(jdbc/postgres): add compatibility for GraalVM native image #805

Merged
merged 38 commits into from
Apr 29, 2022

Conversation

suztomo
Copy link
Contributor

@suztomo suztomo commented Apr 13, 2022

Adding GraalVM native image test. For now, only for jdbc/postgres module passes. (Followup: #824 )

Corresponding change in Kokoro job config: cl/441468021

Change Description

This pull request sets up GraalVM native image tests. For now, only for jdbc/postgres module passes. (Followup: #824 )

Checklist

  • Make sure to open an issue as a
    bug/issue
    before writing your code! That way we can discuss the change, evaluate
    designs, and agree on the general idea.
  • Ensure the tests and linter pass
  • Appropriate documentation is updated (if necessary)

Relevant issues:

@suztomo suztomo changed the title Setting up Kokoro GraalVM native image test ci: setting up Kokoro GraalVM native image test Apr 13, 2022
@suztomo suztomo marked this pull request as draft April 13, 2022 14:20
@suztomo
Copy link
Contributor Author

suztomo commented Apr 13, 2022

Converting to draft until Kokoro GraalVM Native test passes.

@kurtisvg kurtisvg requested a review from shubha-rajan April 13, 2022 14:44
pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
.kokoro/tests/run_tests_graalvm_native.sh Outdated Show resolved Hide resolved
@suztomo
Copy link
Contributor Author

suztomo commented Apr 13, 2022

Kokoro started to receive the job.

image

CC: @mpeddada1

@suztomo
Copy link
Contributor Author

suztomo commented Apr 13, 2022

Only "jdbc/postgres" succeeded out of jdbc/postgres jdbc/mysql-j-5 jdbc/mysql-j-8 jdbc/sqlserver r2dbc/sqlserver r2dbc/sqlserver r2dbc/mysql.

@suztomo
Copy link
Contributor Author

suztomo commented Apr 14, 2022

Moving native-image-support to gax throws these exceptions:

Failures (2):
  JUnit Vintage:JdbcPostgresIamAuthIntegrationTests:pooledConnectionTest
    MethodSource [className = 'com.google.cloud.sql.postgres.JdbcPostgresIamAuthIntegrationTests', methodName = 'pooledConnectionTest', methodParameterTypes = '']
    => org.opentest4j.MultipleFailuresError: Multiple Failures (2 failures)
	com.zaxxer.hikari.pool.HikariPool$PoolInitializationException: Failed to initialize pool: null
	java.lang.NullPointerException: <no message>
       org.junit.vintage.engine.execution.TestRun.getStoredResultOrSuccessful(TestRun.java:196)
       org.junit.vintage.engine.execution.RunListenerAdapter.fireExecutionFinished(RunListenerAdapter.java:226)
       org.junit.vintage.engine.execution.RunListenerAdapter.testFinished(RunListenerAdapter.java:192)
       org.junit.vintage.engine.execution.RunListenerAdapter.testFinished(RunListenerAdapter.java:79)
       org.junit.runner.notification.SynchronizedRunListener.testFinished(SynchronizedRunListener.java:87)
       [...]
       Suppressed: com.zaxxer.hikari.pool.HikariPool$PoolInitializationException: Failed to initialize pool: null
         com.zaxxer.hikari.pool.HikariPool.throwPoolInitializationException(HikariPool.java:596)
         com.zaxxer.hikari.pool.HikariPool.checkFailFast(HikariPool.java:582)
         com.zaxxer.hikari.pool.HikariPool.<init>(HikariPool.java:115)
         com.zaxxer.hikari.HikariDataSource.<init>(HikariDataSource.java:81)
         com.google.cloud.sql.postgres.JdbcPostgresIamAuthIntegrationTests.setUpPool(JdbcPostgresIamAuthIntegrationTests.java:91)
         [...]
       Caused by: java.lang.NullPointerException
         com.zaxxer.hikari.pool.PoolBase.newConnection(PoolBase.java:364)
         com.zaxxer.hikari.pool.PoolBase.newPoolEntry(PoolBase.java:206)
         com.zaxxer.hikari.pool.HikariPool.createPoolEntry(HikariPool.java:476)
         com.zaxxer.hikari.pool.HikariPool.checkFailFast(HikariPool.java:561)
         [...]
       Suppressed: java.lang.NullPointerException
         com.google.cloud.sql.postgres.JdbcPostgresIamAuthIntegrationTests.dropTableIfPresent(JdbcPostgresIamAuthIntegrationTests.java:111)
         java.lang.reflect.Method.invoke(Method.java:566)
         org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
         org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
         org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
         [...]
  JUnit Vintage:JdbcPostgresIntegrationTests:pooledConnectionTest
    MethodSource [className = 'com.google.cloud.sql.postgres.JdbcPostgresIntegrationTests', methodName = 'pooledConnectionTest', methodParameterTypes = '']
    => org.opentest4j.MultipleFailuresError: Multiple Failures (2 failures)
	com.zaxxer.hikari.pool.HikariPool$PoolInitializationException: Failed to initialize pool: null
	java.lang.NullPointerException: <no message>
       org.junit.vintage.engine.execution.TestRun.getStoredResultOrSuccessful(TestRun.java:196)
       org.junit.vintage.engine.execution.RunListenerAdapter.fireExecutionFinished(RunListenerAdapter.java:226)
       org.junit.vintage.engine.execution.RunListenerAdapter.testFinished(RunListenerAdapter.java:192)
       org.junit.vintage.engine.execution.RunListenerAdapter.testFinished(RunListenerAdapter.java:79)
       org.junit.runner.notification.SynchronizedRunListener.testFinished(SynchronizedRunListener.java:87)
       [...]
       Suppressed: com.zaxxer.hikari.pool.HikariPool$PoolInitializationException: Failed to initialize pool: null
         com.zaxxer.hikari.pool.HikariPool.throwPoolInitializationException(HikariPool.java:596)
         com.zaxxer.hikari.pool.HikariPool.checkFailFast(HikariPool.java:582)
         com.zaxxer.hikari.pool.HikariPool.<init>(HikariPool.java:115)
         com.zaxxer.hikari.HikariDataSource.<init>(HikariDataSource.java:81)
         com.google.cloud.sql.postgres.JdbcPostgresIntegrationTests.setUpPool(JdbcPostgresIntegrationTests.java:86)
         [...]
       Caused by: java.lang.NullPointerException
         com.zaxxer.hikari.pool.PoolBase.newConnection(PoolBase.java:364)
         com.zaxxer.hikari.pool.PoolBase.newPoolEntry(PoolBase.java:206)
         com.zaxxer.hikari.pool.HikariPool.createPoolEntry(HikariPool.java:476)
         com.zaxxer.hikari.pool.HikariPool.checkFailFast(HikariPool.java:561)
         [...]
       Suppressed: java.lang.NullPointerException
         com.google.cloud.sql.postgres.JdbcPostgresIntegrationTests.dropTableIfPresent(JdbcPostgresIntegrationTests.java:104)
         java.lang.reflect.Method.invoke(Method.java:566)
         org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
         org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
         org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
         [...]

Todo: Is there Cloud SQL-specific configuration in native-image-support, but not in GAX?

suztomo added 2 commits April 18, 2022 15:08
CloudSqlFeature used to be in a different repository (
https://github.com/GoogleCloudPlatform/native-image-support-java).
Moving the class to this file because t's better to move the
GraalVM native-image configuration to the configured class.
pom.xml Outdated
<assembly.skipAssembly>true</assembly.skipAssembly>
</properties>

<dependencyManagement>
<!-- Forcing the versions for Enforcer's dependency convergence rule -->
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'll think about what to do with these dependency convergence error.

Without these settings, the enforcer rule fails:

[INFO] --- maven-enforcer-plugin:3.0.0:enforce (enforce) @ cloud-sql-connector-r2dbc-core ---
Downloading from oss-sonatype-snapshots: https://oss.sonatype.org/content/repositories/snapshots/com/google/cloud/sql/cloud-sql-connector-r2dbc-core/1.5.1-SNAPSHOT/maven-metadata.xml
[WARNING] 
Dependency convergence error for org.graalvm.nativeimage:svm:jar:22.0.0.2:provided paths to dependency are:
+-com.google.cloud.sql:cloud-sql-connector-r2dbc-core:jar:1.5.1-SNAPSHOT
  +-com.google.cloud.sql:jdbc-socket-factory-core:jar:1.5.1-SNAPSHOT:compile
    +-org.graalvm.nativeimage:svm:jar:22.0.0.2:provided
and
+-com.google.cloud.sql:cloud-sql-connector-r2dbc-core:jar:1.5.1-SNAPSHOT
  +-io.netty:netty-handler:jar:4.1.75.Final:compile
    +-io.netty:netty-common:jar:4.1.75.Final:compile
      +-org.graalvm.nativeimage:svm:jar:19.3.6:provided

pom.xml Outdated
Comment on lines 95 to 96
<artifactId>svm</artifactId>
<version>22.0.0.2</version>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this, the enforcer fails:

[WARNING] 
Dependency convergence error for org.graalvm.nativeimage:svm:jar:22.0.0.2:provided paths to dependency are:
+-com.google.cloud.sql:cloud-sql-connector-r2dbc-core:jar:1.5.1-SNAPSHOT
  +-com.google.cloud.sql:jdbc-socket-factory-core:jar:1.5.1-SNAPSHOT:compile
    +-org.graalvm.nativeimage:svm:jar:22.0.0.2:provided
and
+-com.google.cloud.sql:cloud-sql-connector-r2dbc-core:jar:1.5.1-SNAPSHOT
  +-io.netty:netty-handler:jar:4.1.75.Final:compile
    +-io.netty:netty-common:jar:4.1.75.Final:compile
      +-org.graalvm.nativeimage:svm:jar:19.3.6:provided

pom.xml Outdated
Comment on lines 90 to 91
<artifactId>protobuf-java</artifactId>
<version>3.19.4</version>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this, enforcer fails:

Dependency convergence error for com.google.protobuf:protobuf-java:jar:3.19.4:compile paths to dependency are:
+-com.google.cloud.sql:mysql-socket-factory-connector-j-8:jar:1.5.1-SNAPSHOT
  +-com.google.cloud.sql:jdbc-socket-factory-core:jar:1.5.1-SNAPSHOT:compile
    +-com.google.api:gax:jar:2.16.0:compile
      +-com.google.api.grpc:proto-google-common-protos:jar:2.8.3:compile
        +-com.google.protobuf:protobuf-java:jar:3.19.4:compile
and
+-com.google.cloud.sql:mysql-socket-factory-connector-j-8:jar:1.5.1-SNAPSHOT
  +-mysql:mysql-connector-java:jar:8.0.17:provided
    +-com.google.protobuf:protobuf-java:jar:3.6.1:compile

pom.xml Outdated
Comment on lines 85 to 86
<artifactId>opencensus-api</artifactId>
<version>0.31.0</version>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this, the enforcer fails:

Dependency convergence error for io.opencensus:opencensus-api:jar:0.31.0:compile paths to dependency are:
+-com.google.cloud.sql:mysql-socket-factory:jar:1.5.1-SNAPSHOT
  +-com.google.cloud.sql:jdbc-socket-factory-core:jar:1.5.1-SNAPSHOT:compile
    +-com.google.http-client:google-http-client:jar:1.41.6:compile
      +-io.opencensus:opencensus-api:jar:0.31.0:compile
and
+-com.google.cloud.sql:mysql-socket-factory:jar:1.5.1-SNAPSHOT
  +-com.google.cloud.sql:jdbc-socket-factory-core:jar:1.5.1-SNAPSHOT:compile
    +-com.google.http-client:google-http-client:jar:1.41.6:compile
      +-io.opencensus:opencensus-contrib-http-util:jar:0.31.0:compile
        +-io.opencensus:opencensus-api:jar:0.31.0:compile
and
+-com.google.cloud.sql:mysql-socket-factory:jar:1.5.1-SNAPSHOT
  +-com.google.cloud.sql:jdbc-socket-factory-core:jar:1.5.1-SNAPSHOT:compile
    +-com.google.api:gax:jar:2.16.0:compile
      +-io.opencensus:opencensus-api:jar:0.28.0:runtime

@shubha-rajan
Copy link
Contributor

@shubha-rajan What do you think about limiting the scope this PR to jdbc/postgres and I deal with other 7 modules in separate pull requests?

I think that's a good plan for now! It'll get our build passing while you figure out how to get the other modules working

@suztomo
Copy link
Contributor Author

suztomo commented Apr 27, 2022

"Code Coverage / Coverage check" failed:

BASE BRANCH CODE COVERAGE is 58%
PULL REQUEST CODE COVERAGE is 53%

Comment on lines +313 to 330
<id>prepare-agent</id>
<goals>
<goal>prepare-agent</goal>
</goals>
</execution>
<execution>
<id>report</id>
<phase>test</phase>
<goals>
<goal>report</goal>
</goals>
<configuration>
<excludes>
<!-- Native image test is not part of coverage execution -->
<exclude>com/google/cloud/sql/nativeimage/*</exclude>
</excludes>
</configuration>
</execution>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Fixed strange indent) The real change is adding exclusion for com/google/cloud/sql/nativeimage package.

@suztomo suztomo changed the title ci: setting up Kokoro GraalVM native image test ci: setting up Kokoro GraalVM native image test (jdbc/postgres) Apr 27, 2022
@suztomo
Copy link
Contributor Author

suztomo commented Apr 27, 2022

[kokoro] GraalVM Native / Linux — Internal CI build successful

@suztomo
Copy link
Contributor Author

suztomo commented Apr 27, 2022

@shubha-rajan Thank you for response. Now this PR limits the scope to jdbc/postgres. PTAL.

@suztomo suztomo marked this pull request as ready for review April 27, 2022 14:45
@suztomo suztomo requested a review from shubha-rajan April 27, 2022 14:56
@kurtisvg kurtisvg changed the title ci: setting up Kokoro GraalVM native image test (jdbc/postgres) chore(ci): setting up GraalVM native image test for jdbc/postgres Apr 27, 2022
for test_directory in jdbc/postgres; do
pushd ${test_directory}
echo -e "******************** Running tests in ${test_directory} ********************\n"
mvn -e -B clean verify -P e2e,native -Dcheckstyle.skip -Denforcer.skip
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need checkstyle.skip and enforcer.skip? aren't those already scoped to a lint step?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without -Denforcer.skip, the test-scoped dependency fails in native profile:

[INFO] --- maven-enforcer-plugin:3.0.0:enforce (enforce) @ jdbc-socket-factory-parent ---
[WARNING] 
Dependency convergence error for org.junit.platform:junit-platform-commons:jar:1.8.2:test paths to dependency are:
+-com.google.cloud.sql:jdbc-socket-factory-parent:pom:1.5.1-SNAPSHOT
  +-org.junit.vintage:junit-vintage-engine:jar:5.8.2:test
    +-org.junit.platform:junit-platform-engine:jar:1.8.2:test
      +-org.junit.platform:junit-platform-commons:jar:1.8.2:test
and
+-com.google.cloud.sql:jdbc-socket-factory-parent:pom:1.5.1-SNAPSHOT
  +-org.graalvm.buildtools:junit-platform-native:jar:0.9.11:test
    +-org.junit.jupiter:junit-jupiter:jar:5.8.1:test
      +-org.junit.jupiter:junit-jupiter-api:jar:5.8.1:test
        +-org.junit.platform:junit-platform-commons:jar:1.8.1:test

[WARNING] 
Dependency convergence error for org.junit.platform:junit-platform-engine:jar:1.8.2:test paths to dependency are:
+-com.google.cloud.sql:jdbc-socket-factory-parent:pom:1.5.1-SNAPSHOT
  +-org.junit.vintage:junit-vintage-engine:jar:5.8.2:test
    +-org.junit.platform:junit-platform-engine:jar:1.8.2:test
and
+-com.google.cloud.sql:jdbc-socket-factory-parent:pom:1.5.1-SNAPSHOT
  +-org.graalvm.buildtools:junit-platform-native:jar:0.9.11:test
    +-org.junit.platform:junit-platform-launcher:jar:1.8.1:test
      +-org.junit.platform:junit-platform-engine:jar:1.8.1:test
and
+-com.google.cloud.sql:jdbc-socket-factory-parent:pom:1.5.1-SNAPSHOT
  +-org.graalvm.buildtools:junit-platform-native:jar:0.9.11:test
    +-org.junit.jupiter:junit-jupiter:jar:5.8.1:test
      +-org.junit.jupiter:junit-jupiter-engine:jar:5.8.1:test
        +-org.junit.platform:junit-platform-engine:jar:1.8.1:test

The -Dcheckstyle.skip is from the existing run_tests.sh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a source-code comment to explain that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer we'd rather:

  1. Remove -Dcheckstyle.skip, as we move that functionality to a profile, so I think this is just a leftover from that
  2. Remove -Denforcer.skip and replace it with a dependencyManagement section for the native profile

I'll approve for now but count on @shubha-rajan to make sure the dependencyManagement section gets updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the guide. The two flags are now removed. A dependencyManagement section is added to native profile. 7b73562

core/pom.xml Outdated Show resolved Hide resolved
core/pom.xml Outdated Show resolved Hide resolved
@suztomo suztomo requested a review from kurtisvg April 27, 2022 17:27
@kurtisvg kurtisvg changed the title chore(ci): setting up GraalVM native image test for jdbc/postgres feat(jdbc/postgres): add compatibility for GraalVM native image Apr 28, 2022
for test_directory in jdbc/postgres; do
pushd ${test_directory}
echo -e "******************** Running tests in ${test_directory} ********************\n"
mvn -e -B clean verify -P e2e,native -Dcheckstyle.skip -Denforcer.skip
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer we'd rather:

  1. Remove -Dcheckstyle.skip, as we move that functionality to a profile, so I think this is just a leftover from that
  2. Remove -Denforcer.skip and replace it with a dependencyManagement section for the native profile

I'll approve for now but count on @shubha-rajan to make sure the dependencyManagement section gets updated.

Comment on lines 106 to 116
<exclusions>
<exclusion>
<!--
Because this svn dependency is provided-scope, the users of this
library don't get this svn dependency anyway. Explicitly excluding
this dependency for the dependencyConvergence enforcer rule.
-->
<groupId>org.graalvm.nativeimage</groupId>
<artifactId>svm</artifactId>
</exclusion>
</exclusions>
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we scoped this to just jdbc/postgres for now, should we revert the changes to other files until you update them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted the changes under r2dbc modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But they needed to have svm elements for dependencyManagement sections.

ea0adcc

shubha-rajan
shubha-rajan previously approved these changes Apr 29, 2022
@shubha-rajan shubha-rajan dismissed their stale review April 29, 2022 17:59

Waiting for updates to dependencyManagement

@shubha-rajan shubha-rajan added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 29, 2022
Copy link
Contributor Author

@suztomo suztomo left a comment

Choose a reason for hiding this comment

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

Comment on lines +368 to +382
<dependencyManagement>
<!-- Controlling version for dependencyConvergence enforcer rule -->
<dependencies>
<dependency>
<groupId>org.junit.platform</groupId>
<artifactId>junit-platform-engine</artifactId>
<version>1.8.2</version>
</dependency>
<dependency>
<groupId>org.junit.platform</groupId>
<artifactId>junit-platform-commons</artifactId>
<version>1.8.2</version>
</dependency>
</dependencies>
</dependencyManagement>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now "native" profile has this dependencyManagement section to control the enforcer rule.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Thanks for doing that.

@shubha-rajan shubha-rajan removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 29, 2022
@shubha-rajan
Copy link
Contributor

LGTM 🎉

@shubha-rajan shubha-rajan merged commit c00c255 into GoogleCloudPlatform:main Apr 29, 2022
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.

GraalVM native image test as CI
3 participants