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

fix: grpc-alts is used not only in tests #761

Merged
merged 3 commits into from Jan 5, 2021

Conversation

thiagotnunes
Copy link
Contributor

@thiagotnunes thiagotnunes commented Dec 21, 2020

This reverts commit c8ef46f.

Without having grpc-alts in runtime / compile scope I get the following error when running the client library:

Exception in thread "main" java.lang.NoClassDefFoundError: io/grpc/alts/ComputeEngineChannelBuilder
	at com.google.cloud.spanner.v1.stub.SpannerStubSettings.defaultGrpcTransportProviderBuilder(SpannerStubSettings.java:304)
	at com.google.cloud.spanner.v1.stub.SpannerStubSettings.defaultTransportChannelProvider(SpannerStubSettings.java:309)
	at com.google.cloud.spanner.v1.stub.SpannerStubSettings$Builder.createDefault(SpannerStubSettings.java:531)
	at com.google.cloud.spanner.v1.stub.SpannerStubSettings$Builder.access$100(SpannerStubSettings.java:356)
	at com.google.cloud.spanner.v1.stub.SpannerStubSettings.newBuilder(SpannerStubSettings.java:322)
	at com.google.cloud.spanner.SpannerOptions$Builder.<init>(SpannerOptions.java:628)
	at com.google.cloud.spanner.SpannerOptions$Builder.<init>(SpannerOptions.java:605)
	at com.google.cloud.spanner.SpannerOptions.newBuilder(SpannerOptions.java:1020)
	at com.google.cloud.spanner.Main.main(Main.java:33)
Caused by: java.lang.ClassNotFoundException: io.grpc.alts.ComputeEngineChannelBuilder
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:583)
	at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)
	... 9 more

@thiagotnunes thiagotnunes requested review from as code owners Dec 21, 2020
@product-auto-label product-auto-label bot added the api: spanner label Dec 21, 2020
@google-cla google-cla bot added the cla: yes label Dec 21, 2020
@thiagotnunes thiagotnunes requested a review from olavloite Dec 21, 2020
@thiagotnunes
Copy link
Contributor Author

@thiagotnunes thiagotnunes commented Dec 21, 2020

@codecov
Copy link

@codecov codecov bot commented Dec 21, 2020

Codecov Report

Merging #761 (874ec0c) into master (c8ef46f) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #761   +/-   ##
=========================================
  Coverage     84.99%   85.00%           
  Complexity     2562     2562           
=========================================
  Files           143      143           
  Lines         13983    14007   +24     
  Branches       1335     1338    +3     
=========================================
+ Hits          11885    11906   +21     
- Misses         1537     1538    +1     
- Partials        561      563    +2     
Impacted Files Coverage Δ Complexity Δ
...a/com/google/cloud/spanner/SpannerRetryHelper.java 77.27% <0.00%> (-4.55%) 3.00% <0.00%> (-1.00%)
...gle/cloud/spanner/testing/RemoteSpannerHelper.java 0.00% <0.00%> (ø) 0.00% <0.00%> (ø%)
...ain/java/com/google/cloud/spanner/SessionImpl.java 85.88% <0.00%> (+0.08%) 30.00% <0.00%> (ø%)
.../java/com/google/cloud/spanner/SpannerOptions.java 89.54% <0.00%> (+0.20%) 40.00% <0.00%> (+1.00%)
...om/google/cloud/spanner/TransactionRunnerImpl.java 84.70% <0.00%> (+0.33%) 9.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8ef46f...874ec0c. Read the comment docs.

<dependency>
<groupId>io.grpc</groupId>
<artifactId>grpc-alts</artifactId>
<scope>runtime</scope>
Copy link
Contributor

@olavloite olavloite Dec 21, 2020

Choose a reason for hiding this comment

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

grpc-alts is already a compile time dependency as it is a transitive dependency of gax-grpc. This degrades it to a runtime dependency. Is there a reason that we need to downgrade it here, and that we can't just let it be a compile time dependency?

Suggested change
<scope>runtime</scope>

This is the dependency tree completely without the grpc-alts dependency declaration:

[INFO] Scanning for projects...
[INFO] Inspecting build with total of 1 modules...
[INFO] Installing Nexus Staging features:
[INFO]   ... total of 1 executions of maven-deploy-plugin replaced with nexus-staging-maven-plugin
[INFO] 
[INFO] ---------------< com.google.cloud:google-cloud-spanner >----------------
[INFO] Building Google Cloud Spanner 3.1.4-SNAPSHOT
[INFO] --------------------------------[ jar ]---------------------------------
[INFO] 
[INFO] --- maven-dependency-plugin:3.1.2:tree (default-cli) @ google-cloud-spanner ---
[INFO] com.google.cloud:google-cloud-spanner:jar:3.1.4-SNAPSHOT
[INFO] +- io.grpc:grpc-api:jar:1.34.1:compile
[INFO] |  +- com.google.errorprone:error_prone_annotations:jar:2.4.0:compile
[INFO] |  \- org.codehaus.mojo:animal-sniffer-annotations:jar:1.19:runtime
[INFO] +- io.grpc:grpc-auth:jar:1.34.1:compile
[INFO] +- io.grpc:grpc-context:jar:1.34.1:compile
[INFO] +- io.grpc:grpc-core:jar:1.34.1:compile
[INFO] |  +- com.google.android:annotations:jar:4.1.1.4:runtime
[INFO] |  \- io.perfmark:perfmark-api:jar:0.19.0:runtime
[INFO] +- io.grpc:grpc-netty-shaded:jar:1.34.1:compile
[INFO] +- io.grpc:grpc-protobuf:jar:1.34.1:compile
[INFO] |  \- io.grpc:grpc-protobuf-lite:jar:1.34.1:compile
[INFO] +- io.grpc:grpc-stub:jar:1.34.1:compile
[INFO] +- com.google.api:api-common:jar:1.10.1:compile
[INFO] |  \- com.google.auto.value:auto-value-annotations:jar:1.7.4:compile
[INFO] +- com.google.protobuf:protobuf-java:jar:3.14.0:compile
[INFO] +- com.google.protobuf:protobuf-java-util:jar:3.14.0:compile
[INFO] +- com.google.api.grpc:proto-google-common-protos:jar:2.0.1:compile
[INFO] +- com.google.api.grpc:grpc-google-common-protos:jar:2.0.1:compile
[INFO] |  +- com.google.guava:failureaccess:jar:1.0.1:compile
[INFO] |  +- com.google.guava:listenablefuture:jar:9999.0-empty-to-avoid-conflict-with-guava:compile
[INFO] |  +- org.checkerframework:checker-compat-qual:jar:2.5.5:compile
[INFO] |  \- com.google.j2objc:j2objc-annotations:jar:1.3:compile
[INFO] +- com.google.api.grpc:proto-google-iam-v1:jar:1.0.5:compile
[INFO] +- com.google.cloud:google-cloud-core:jar:1.94.0:compile
[INFO] |  \- com.google.http-client:google-http-client-jackson2:jar:1.38.0:compile
[INFO] |     \- com.fasterxml.jackson.core:jackson-core:jar:2.12.0:compile
[INFO] +- com.google.cloud:google-cloud-core-grpc:jar:1.94.0:compile
[INFO] +- io.opencensus:opencensus-api:jar:0.24.0:compile
[INFO] +- io.opencensus:opencensus-contrib-grpc-util:jar:0.24.0:compile
[INFO] +- com.google.auth:google-auth-library-oauth2-http:jar:0.22.2:compile
[INFO] +- com.google.http-client:google-http-client:jar:1.38.0:compile
[INFO] |  +- org.apache.httpcomponents:httpclient:jar:4.5.13:compile
[INFO] |  |  +- commons-logging:commons-logging:jar:1.2:compile
[INFO] |  |  \- commons-codec:commons-codec:jar:1.11:compile
[INFO] |  +- org.apache.httpcomponents:httpcore:jar:4.4.13:compile
[INFO] |  \- io.opencensus:opencensus-contrib-http-util:jar:0.24.0:compile
[INFO] +- com.google.api.grpc:proto-google-cloud-spanner-admin-instance-v1:jar:3.1.4-SNAPSHOT:compile
[INFO] +- com.google.api.grpc:proto-google-cloud-spanner-v1:jar:3.1.4-SNAPSHOT:compile
[INFO] +- com.google.api.grpc:proto-google-cloud-spanner-admin-database-v1:jar:3.1.4-SNAPSHOT:compile
[INFO] +- com.google.api.grpc:grpc-google-cloud-spanner-admin-instance-v1:jar:3.1.4-SNAPSHOT:compile
[INFO] +- com.google.api.grpc:grpc-google-cloud-spanner-v1:jar:3.1.4-SNAPSHOT:compile
[INFO] +- com.google.api.grpc:grpc-google-cloud-spanner-admin-database-v1:jar:3.1.4-SNAPSHOT:compile
[INFO] +- com.google.guava:guava:jar:30.1-android:compile
[INFO] +- com.google.api:gax:jar:1.60.1:compile
[INFO] +- com.google.api:gax-grpc:jar:1.60.1:compile
[INFO] |  \- io.grpc:grpc-alts:jar:1.34.1:compile
[INFO] |     +- io.grpc:grpc-grpclb:jar:1.34.1:compile
[INFO] |     +- org.apache.commons:commons-lang3:jar:3.5:compile
[INFO] |     \- org.conscrypt:conscrypt-openjdk-uber:jar:2.5.1:compile
[INFO] +- org.threeten:threetenbp:jar:1.5.0:compile
[INFO] +- com.google.code.findbugs:jsr305:jar:3.0.2:compile
[INFO] +- com.google.code.gson:gson:jar:2.8.6:compile
[INFO] +- com.google.auth:google-auth-library-credentials:jar:0.22.2:compile
[INFO] +- junit:junit:jar:4.13.1:test
[INFO] |  \- org.hamcrest:hamcrest-core:jar:1.3:test
[INFO] +- com.google.api:gax-grpc:jar:testlib:1.60.1:test
[INFO] +- com.google.truth:truth:jar:1.1:test
[INFO] |  +- org.checkerframework:checker-qual:jar:3.7.0:test
[INFO] |  \- org.ow2.asm:asm:jar:9.0:test
[INFO] +- org.mockito:mockito-all:jar:1.10.19:test
[INFO] +- org.json:json:jar:20201115:test
[INFO] +- com.google.guava:guava-testlib:jar:30.1-android:test
[INFO] +- org.hamcrest:hamcrest:jar:2.2:test
[INFO] +- org.openjdk.jmh:jmh-core:jar:1.27:test
[INFO] |  +- net.sf.jopt-simple:jopt-simple:jar:4.6:test
[INFO] |  \- org.apache.commons:commons-math3:jar:3.2:test
[INFO] +- org.openjdk.jmh:jmh-generator-annprocess:jar:1.27:test
[INFO] \- javax.annotation:javax.annotation-api:jar:1.3.2:compile
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 2.470 s
[INFO] Finished at: 2020-12-21T08:17:23+01:00
[INFO] ------------------------------------------------------------------------

@thiagotnunes
Copy link
Contributor Author

@thiagotnunes thiagotnunes commented Dec 21, 2020

@olavloite by removing the grpc-alts from the pom.xml, everything works as expected. However, by having it as a test dependency (test scoped), I do get the exception above and can not run the library. This is the code I am using for testing:

final SpannerOptions options = SpannerOptions.newBuilder().build();
final Spanner spanner = options.getService();
final DatabaseAdminClient databaseAdminClient = spanner.getDatabaseAdminClient();

final OperationFuture<Database, CreateDatabaseMetadata> op = databaseAdminClient
  .createDatabase(
    INSTANCE,
    DATABASE,
    Collections.<String>emptyList()
  );
op.get();

If you look at the dependency tree with the grpc-alts as a test dependency, you can see it to be different than the one you had above:

[INFO] com.google.cloud:google-cloud-spanner:jar:3.2.0
[INFO] +- io.grpc:grpc-api:jar:1.34.1:compile
[INFO] |  +- com.google.errorprone:error_prone_annotations:jar:2.4.0:compile
[INFO] |  \- org.codehaus.mojo:animal-sniffer-annotations:jar:1.19:runtime
[INFO] +- io.grpc:grpc-auth:jar:1.34.1:compile
[INFO] +- io.grpc:grpc-context:jar:1.34.1:compile
[INFO] +- io.grpc:grpc-core:jar:1.34.1:compile
[INFO] |  +- com.google.android:annotations:jar:4.1.1.4:runtime
[INFO] |  \- io.perfmark:perfmark-api:jar:0.19.0:runtime
[INFO] +- io.grpc:grpc-netty-shaded:jar:1.34.1:compile
[INFO] +- io.grpc:grpc-protobuf:jar:1.34.1:compile
[INFO] |  \- io.grpc:grpc-protobuf-lite:jar:1.34.1:compile
[INFO] +- io.grpc:grpc-stub:jar:1.34.1:compile
[INFO] +- com.google.api:api-common:jar:1.10.1:compile
[INFO] |  +- javax.annotation:javax.annotation-api:jar:1.3.2:compile
[INFO] |  \- com.google.auto.value:auto-value-annotations:jar:1.7.4:compile
[INFO] +- com.google.protobuf:protobuf-java:jar:3.14.0:compile
[INFO] +- com.google.protobuf:protobuf-java-util:jar:3.14.0:compile
[INFO] +- com.google.api.grpc:proto-google-common-protos:jar:2.0.1:compile
[INFO] +- com.google.api.grpc:grpc-google-common-protos:jar:2.0.1:compile
[INFO] |  +- com.google.guava:failureaccess:jar:1.0.1:compile
[INFO] |  +- com.google.guava:listenablefuture:jar:9999.0-empty-to-avoid-conflict-with-guava:compile
[INFO] |  +- org.checkerframework:checker-compat-qual:jar:2.5.5:compile
[INFO] |  \- com.google.j2objc:j2objc-annotations:jar:1.3:compile
[INFO] +- com.google.api.grpc:proto-google-iam-v1:jar:1.0.5:compile
[INFO] +- com.google.cloud:google-cloud-core:jar:1.94.0:compile
[INFO] |  \- com.google.http-client:google-http-client-jackson2:jar:1.38.0:compile
[INFO] |     \- com.fasterxml.jackson.core:jackson-core:jar:2.12.0:compile
[INFO] +- com.google.cloud:google-cloud-core-grpc:jar:1.94.0:compile
[INFO] +- io.opencensus:opencensus-api:jar:0.24.0:compile
[INFO] +- io.opencensus:opencensus-contrib-grpc-util:jar:0.24.0:compile
[INFO] +- com.google.auth:google-auth-library-oauth2-http:jar:0.22.2:compile
[INFO] +- com.google.http-client:google-http-client:jar:1.38.0:compile
[INFO] |  +- org.apache.httpcomponents:httpclient:jar:4.5.13:compile
[INFO] |  |  +- commons-logging:commons-logging:jar:1.2:compile
[INFO] |  |  \- commons-codec:commons-codec:jar:1.11:compile
[INFO] |  +- org.apache.httpcomponents:httpcore:jar:4.4.13:compile
[INFO] |  \- io.opencensus:opencensus-contrib-http-util:jar:0.24.0:compile
[INFO] +- com.google.api.grpc:proto-google-cloud-spanner-admin-instance-v1:jar:3.2.0:compile
[INFO] +- com.google.api.grpc:proto-google-cloud-spanner-v1:jar:3.2.0:compile
[INFO] +- com.google.api.grpc:proto-google-cloud-spanner-admin-database-v1:jar:3.2.0:compile
[INFO] +- com.google.api.grpc:grpc-google-cloud-spanner-admin-instance-v1:jar:3.2.0:compile
[INFO] +- com.google.api.grpc:grpc-google-cloud-spanner-v1:jar:3.2.0:compile
[INFO] +- com.google.api.grpc:grpc-google-cloud-spanner-admin-database-v1:jar:3.2.0:compile
[INFO] +- com.google.guava:guava:jar:30.1-android:compile
[INFO] +- com.google.api:gax:jar:1.60.1:compile
[INFO] +- com.google.api:gax-grpc:jar:1.60.1:compile
[INFO] +- org.threeten:threetenbp:jar:1.5.0:compile
[INFO] +- com.google.code.findbugs:jsr305:jar:3.0.2:compile
[INFO] +- com.google.code.gson:gson:jar:2.8.6:compile
[INFO] +- com.google.auth:google-auth-library-credentials:jar:0.22.2:compile
[INFO] +- junit:junit:jar:4.13.1:test
[INFO] |  \- org.hamcrest:hamcrest-core:jar:1.3:test
[INFO] +- io.grpc:grpc-alts:jar:1.34.1:test
[INFO] |  +- io.grpc:grpc-grpclb:jar:1.34.1:test
[INFO] |  +- org.apache.commons:commons-lang3:jar:3.5:test
[INFO] |  \- org.conscrypt:conscrypt-openjdk-uber:jar:2.5.1:test
[INFO] +- com.google.api:gax-grpc:jar:testlib:1.60.1:test
[INFO] +- com.google.truth:truth:jar:1.1:test
[INFO] |  +- org.checkerframework:checker-qual:jar:3.7.0:test
[INFO] |  \- org.ow2.asm:asm:jar:9.0:test
[INFO] +- org.mockito:mockito-all:jar:1.10.19:test
[INFO] +- org.json:json:jar:20201115:test
[INFO] +- com.google.guava:guava-testlib:jar:30.1-android:test
[INFO] +- org.hamcrest:hamcrest:jar:2.2:test
[INFO] +- org.openjdk.jmh:jmh-core:jar:1.27:test
[INFO] |  +- net.sf.jopt-simple:jopt-simple:jar:4.6:test
[INFO] |  \- org.apache.commons:commons-math3:jar:3.2:test
[INFO] \- org.openjdk.jmh:jmh-generator-annprocess:jar:1.27:test

As you can see above, alts is only pulled as a test dependency.

@olavloite
Copy link
Contributor

@olavloite olavloite commented Dec 22, 2020

by removing the grpc-alts from the pom.xml, everything works as expected. However, by having it as a test dependency (test scoped), I do get the exception above and can not run the library.

Yes, it seems to be needed as a compile time dependency. Removing it from the pom.xml will make it work locally, but it will also break the dependencies check, as the latter requires all (transitive) dependencies that are actually used to be explicitly declared in the pom. So the best solution here is to just remove the runtime scope from the dependency declaration.

@thiagotnunes
Copy link
Contributor Author

@thiagotnunes thiagotnunes commented Dec 22, 2020

Added grpc-alts as a compile scope dependency.

Copy link
Contributor

@olavloite olavloite left a comment

The dependency is now included twice in the pom (once as runtime dependency and once as a compile dependency). I think we should only include it once.

Removes duplicate grpc-alts dependency
@thiagotnunes
Copy link
Contributor Author

@thiagotnunes thiagotnunes commented Jan 4, 2021

@olavloite thanks for spotting that, pushed the fix (removed the runtime dependency).

@thiagotnunes thiagotnunes merged commit 72d93d5 into googleapis:master Jan 5, 2021
20 checks passed
@thiagotnunes thiagotnunes deleted the revert-grpc-alts branch Jan 5, 2021
thiagotnunes added a commit that referenced this issue May 6, 2021
* fix: grpc-alts is used not only in tests

This reverts commit c8ef46f.

* fix: adds grpc-alts as compile time dependency

* fix: removes duplicate dependency

Removes duplicate grpc-alts dependency
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants