googleapis / google-cloud-java

Google Cloud Client Library for Java
https://cloud.google.com/java/docs/reference
Apache License 2.0
1.88k stars 1.06k forks source link

Create variant of google-cloud-java with Guava and grpc-stub shaded #1573

Closed garrettjonesgoogle closed 5 years ago

garrettjonesgoogle commented 7 years ago

Note: GAX needs to be exposed.

Consequences:

  1. Guava can't be exposed on the public surface of GAX or google-cloud-java
  2. grpc-stub can't be exposed on the public surface of GAX or google-cloud-java (since it exposes Guava)
  3. The generated grpc packages need to be split into two: a package with proto-generated types (no Guava dependency) and a package with only grpc (which exposes Guava), and the second type of package needs to be shaded.

Coordinating issue in GAX is filed at https://github.com/googleapis/gax-java/issues/188 .

Also: protobuf-java-util needs to be shaded.

@VisibleForTesting needs to be removed throughout the code.

garrettjonesgoogle commented 7 years ago

blocking issues:

garrettjonesgoogle commented 7 years ago

I will migrate Java grpc package generation to toolkit as part of this change.

vkedia commented 7 years ago

We will also need to shade protobuf-java-util and guava that it depends on once PR #1770 is merged.

garrettjonesgoogle commented 7 years ago

The task to split the proto-generated classes and grpc-generated classes is being tracked in https://github.com/googleapis/toolkit/issues/803 , and @shinfan is working on it.

garrettjonesgoogle commented 7 years ago

Actually, the splitting of proto-generated classes and grpc-generated classes is being tracked in https://github.com/googleapis/toolkit/issues/1145 ; googleapis/toolkit#803 is a prerequisite.

garrettjonesgoogle commented 7 years ago

The release-blocking aspect of this issue is to confirm that Guava can be shaded, and that there is no Guava on the surface of any package we own in the stack (api-common-java, gax-java, google-cloud-java). The actual shading support can come after 1.0.

michaelbausor commented 7 years ago

Tested that guava can be shaded by successfully running integration tests against a shaded artifact with Guava 12 loaded. Can also confirm that Guava has been removed from the surface of api-common, gax-java and google-cloud-java

lndbrg commented 6 years ago

This is sorely needed for us. I am currently blocked in a place where I have two conflicting guavas, on in a classloader I can control the other in one i can not control (writing a plugin, that depends on another plugin). Where the plugin I depend upon uses the appllication classloader (and gets guava 11) but the google cloud library I use requires guava 20.

Can you prioritise this higher? Exposing such a widely used dependency that is not binary compatible is a huge mistake.

i11 commented 6 years ago

Hey! Any estimation on when this could go in? We have a few cases where this causes quite a bit of pain.

btilford commented 6 years ago

Same situation here, one library breaks with < guava 20 others break when it's > 20. Basically making it really hard to use GCP. Always been able to work through guava conflicts in 3rd party libraries in the past but sort of stuck with GCP and other google apis.

yihanzhen commented 6 years ago

This has been added to our feature backlog: https://github.com/GoogleCloudPlatform/google-cloud-java/wiki/Feature-backlog . This issue will be closed but is linked in the backlog and can continue to be used for comment and discussion.

lndbrg commented 6 years ago

@hzyi-google any updates on this? it's really hard to use your google cloud libraries together with other dependencies because of this. can this be prioritised higher?

garrettjonesgoogle commented 6 years ago

I separately filed another issue: https://github.com/GoogleCloudPlatform/google-cloud-java/issues/3049 "Make google-cloud-java forward compatible with Guava 23+" which I think will help anyone who's trying to use google-cloud-java with the latest version of Guava. I'm doubtful that we will actually produce a variant of google-cloud-java with Guava shaded - there are a lot of problems with having multiple variants of a library with different numbers of things shaded.

@lndbrg , is your application Hadoop by any chance?

lndbrg commented 6 years ago

@garrettjonesgoogle no, i have since moved on from that project, that current instance was jenkins, but since they can do magic with classpaths it was fine. it's a microservice pulling in different dependencies, no hadoop in this case but it's still unmanageable, the moment you depend on something more than the google cloud libraries, you can expect to get sad and spend a coupe of hours on trying to fix it, sometimes it's doable sometimes it's not. the amount of yak shaving required to get stuff working with google cloud products is becoming unbearable.

preferably you shouldn't depend on guava at all or build a separat repackaged guava for google cloud and use that everywhere and also make sure to never expose it.

garrettjonesgoogle commented 6 years ago

@lndbrg are you able to list any of the other products that google-cloud products are conflicting with?

edmonddc commented 6 years ago

@garrettjonesgoogle OpenTSDB uses guava version 18.

I'm getting this error during runtime:

java.lang.NoClassDefFoundError: Could not initialize class com.google.pubsub.v1.PubsubMessage$AttributesDefaultEntryHolder

garrettjonesgoogle commented 6 years ago

@edmonddc on the surface that doesn't seem related to Guava - could you give a more extended stack trace?

edmonddc commented 6 years ago

@garrettjonesgoogle, I'm not really sure if it is really guava. But I got the idea that it might be guava from this:

https://github.com/GoogleCloudPlatform/google-cloud-java/blob/master/TROUBLESHOOTING.md

This is the stacktrace: 018-04-30 07:55:14,813 ERROR [OpenTSDB I/O Worker #3] ConnectionManager: Unexpected exception from downstream for [id: 0x7ec44c51, /172.18.0.14:59892 :> /172.18.0.39:4242] java.lang.NoClassDefFoundError: Could not initialize class com.google.pubsub.v1.PubsubMessage$AttributesDefaultEntryHolder at com.google.pubsub.v1.PubsubMessage$Builder.internalGetAttributes(PubsubMessage.java:758) ~[GCloudPublisher-0.1.0-jar-with-dependencies.jar:na] at com.google.pubsub.v1.PubsubMessage$Builder.buildPartial(PubsubMessage.java:622) ~[GCloudPublisher-0.1.0-jar-with-dependencies.jar:na] at com.google.pubsub.v1.PubsubMessage$Builder.build(PubsubMessage.java:610) ~[GCloudPublisher-0.1.0-jar-with-dependencies.jar:na] at com.cascadeo.tsdb.GCloudPublisher.publishDataPointHelper(GCloudPublisher.java:177) ~[GCloudPublisher-0.1.0-jar-with-dependencies.jar:na] at com.cascadeo.tsdb.GCloudPublisher.publishDataPoint(GCloudPublisher.java:94) ~[GCloudPublisher-0.1.0-jar-with-dependencies.jar:na] at net.opentsdb.tsd.RTPublisher.sinkDataPoint(RTPublisher.java:105) ~[tsdb-2.3.0.jar:] at net.opentsdb.core.TSDB$1WriteCB.call(TSDB.java:1035) ~[tsdb-2.3.0.jar:] at net.opentsdb.core.TSDB$1WriteCB.call(TSDB.java:977) ~[tsdb-2.3.0.jar:] at com.stumbleupon.async.Deferred.doCall(Deferred.java:1278) ~[async-1.4.0.jar:na] at com.stumbleupon.async.Deferred.addCallbacks(Deferred.java:688) ~[async-1.4.0.jar:na] at com.stumbleupon.async.Deferred.addCallbackDeferring(Deferred.java:738) ~[async-1.4.0.jar:na] at net.opentsdb.core.TSDB.addPointInternal(TSDB.java:1049) ~[tsdb-2.3.0.jar:]

garrettjonesgoogle commented 6 years ago

I don't see Guava anywhere in the stack trace... I do see that your jar is named GCloudPublisher-0.1.0-jar-with-dependencies.jar, implying you have done some shading. Maybe your shading configuration isn't quite right?

edmonddc commented 6 years ago

@garrettjonesgoogle I haven't done any shading yet. But I might do that to fix the runtime issue. I made GCloudPublisher for OpenTSDB as RT plugin.

edmonddc commented 6 years ago

Just dropping this here... I was able to fix the issue on my build. The issue is on protobuf and not on guava. The runtime enviroment on OpenTSDB is using an older version of protobuf. After shading the protobuf on the pom file, I was able to connect to google cloud pubsub.

chingor13 commented 5 years ago

We upgraded to Guava 26.0-android. I don't think this is currently necessary

sduskis commented 5 years ago

@chingor13, we should figure out what to do about the Hadoop ecosystem

garrettjonesgoogle commented 5 years ago

Apparently Hadoop uses Guava 19 now: https://issues.apache.org/jira/browse/HADOOP-14380

sduskis commented 5 years ago

This should probably happen after @chingor13 reorgs the clients.

garrettjonesgoogle commented 5 years ago

I don't think we want to do this any more.