Shade Netty transport #2485

ejona86 · 2016-12-05T18:39:24Z

The entire transport is shaded because we wouldn't want to collide with
the unshaded form of grpc-netty.

jhspaybar · 2016-12-08T18:14:29Z

So this means that gRPC netty will no longer have a chance to conflict with other Netty versions I'm using? If so, +1, we were thinking of doing this ourselves when we imported and used gRPC anyways. The http2 changes between 4.1.5.Final and 4.1.6.Final were frustrating and it happens often enough to cause issues in a big organization.

How will this play with shared things like worker groups and whatnot? If I have normal Netty plus grpc Netty, can I use the same worker group between them, or will I likely be stuck with 2 worker groups due to the relocation of classes?

ejona86 · 2016-12-08T18:32:40Z

Yes, this would remove or reduce the risk of conflicting Netty versions (depending on whether we continue to shade all of Netty, or just a risky subportion).

The two Nettys currently share no code, so you'd have to use different EventLoops. "Normally" we could just shade the HTTP/2 codec. However, the HTTP/2 codec uses code from internal Netty classes that have no API stability. The internal classes then need all of Netty to be shaded. It may be possible to shade just some of the internal classes and the HTTP/2 codec, but it seems a bit dangerous.

jhspaybar · 2016-12-08T19:53:50Z

Okay, that was the outcome we had evaluated as well internally. We feel the risk of breakage due to dependency conflicts is greater than the cost of not being able to use a single shared event loop for all netty's. We are in support of a complete shading of all netty code, not just http2.

nevi-me · 2017-01-09T12:49:41Z

I tried using gRPC with Apache Spark, and it was working well while developing on local Windows machine (running with IntelliJ), but when I try running my fatJar on Apache Spark, I get Netty related issues.

Is this something that shading would be able to resolve?

ejona86 · 2017-01-09T17:32:11Z

I get Netty related issues

That is too vague to tell you anything. If it was Netty version incompatibilities, then yes, shading would likely resolve the problem.

zackangelo · 2017-01-12T21:41:12Z

Netty conflicts have bitten us pretty hard internally during our gRPC pilot. A shaded dependency would be very welcome on our team.

ejona86 · 2017-01-24T22:06:10Z

Jenkins, retest this please.

nevi-me · 2017-01-24T22:19:40Z

That is too vague to tell you anything. If it was Netty version incompatibilities, then yes, shading would likely resolve the problem.

I missed this, I tried reproducing the exact error, but dealing with other nonsense now 😞

Essentially, Spark was complaining about a clash with class names, I know I'm still being vague, but hopefully I'll be able to get someone in my team to reproduce the error in the coming days.

ejona86 · 2017-01-27T20:50:59Z

@nevi-me, if it is similar to googleapis/google-cloud-java#1522 then it would be resolved. The root error there is a NoSuchMethodError, which is caused by version incompatibility.

jhspaybar · 2017-02-02T19:04:00Z

This is becoming a pretty significant source of pain for us. Is this something we think would be included in the 1.2 release(or even a 1.1.x release if possible)? I'd like to determine if this is for sure going to happen so it then becomes a discussion of when, or if we should be shading this ourselves internally if the decision is no. Thanks.

garrettjonesgoogle · 2017-02-09T19:45:53Z

@ejona86 1.0 -> 1.1 is a minor version change, but a deleted method is a breaking change, which is inconsistent it being a minor version change.

kuroneko25 · 2017-02-09T21:56:44Z

+1 on including this in 1.1.x release. This is currently preventing us from upgrading from 1.0 to 1.1.

ejona86 · 2017-03-10T22:23:37Z

@ejona86 1.0 -> 1.1 is a minor version change, but a deleted method is a breaking change, which is inconsistent it being a minor version change.

I assume you meant 4.0 → 4.1. The failure in logic is Netty is not using your semantics for versioning. Netty has lots of interfaces and it's my understanding they reserve the right to change them in minor versions. So upgrading minor versions isn't too hard, but is not 100% API compatible.

zackangelo · 2017-03-21T19:48:08Z

@ejona86 are y'all still considering this? and if so, any idea when it'll land?

jhspaybar · 2017-05-03T17:19:53Z

@ejona86 Can you provide any updates on this? It would be helpful for a decision one way or the other. If gRPC will shade, that is our preference at Netflix, but if gRPC won't shade, we're likely to shade anyways when we import. Right now the limbo state is worst for us, because we don't want to shade and then have you do it anyways :)

nevi-me · 2017-05-03T17:24:48Z

@jhspaybar sorry for the amateur question, how would one shade the current version using Gradle?

jhspaybar · 2017-05-03T17:25:29Z

@nevi-me This thread has an accompanying commit which shows how.

nevi-me · 2017-05-03T17:29:04Z

Silly me, this is a PR haha. Thanks!

ejona86 · 2017-05-03T22:23:49Z

@jhspaybar, as far as decisions go, having a version of grpc-netty that has Netty shaded is a "yes". We'll still have the unshaded one as well (and the two will use different package names). But I need to have time to finish the work. That will be "soon" (weeks).

vkedia · 2017-06-08T20:36:54Z

@ejona86 Any updates on this?

ejona86 · 2017-07-08T00:20:59Z

In netty/netty-tcnative#272, netty-tcnative began supporting shading. This needs to be updated to include tcnative, at which point it should be a fully self-contained one-stop-shop.

ejona86 · 2017-07-12T18:27:55Z

I created netty/netty#6963 for discussing a out-of-the-box alternative to io.netty.packagePrefix.

ejona86 · 2017-07-21T23:20:48Z

Netty can now (in the next release) auto-detect the packagePrefix. That means this PR is very close.

Unfortunately, the shadow plugin is a bit strange when publishing. I need to investigate more how to upload to Maven Central (sanely) and how to install locally (sanely), when using the shadow plugin.

ejona86 · 2017-08-29T02:10:19Z

I'll need to rebase this now that we've updated Netty. The new Netty has minor tweaks to the file names that we need to shade. I hope the test catches that it is broken.

ejona86 · 2017-08-29T02:11:27Z

@carl-mastrangelo, @zhangkun83, please review. I imagine Carl is probably more comfortable with the Netty side and Kun is probably more comfortable with the Gradle side.

carl-mastrangelo · 2017-08-29T19:08:33Z

netty/shaded/build.gradle

+    // We have to be careful with these replacements as they must not match any
+    // string in NativeLibraryLoader, else they cause corruption. Note that
+    // this includes concatenation of string literals and constants.
+    relocate 'META-INF/native/libnetty', 'META-INF/native/libio-grpc-netty-shaded-netty'


These should be underscores. See https://github.com/netty/netty-tcnative/blob/master/openssl-dynamic/src/main/c/jnilib.c#L276

In the version of master this is on, they must be dashes. This is what I as referring to with "I'll need to rebase this now that we've updated Netty. The new Netty has minor tweaks to the file names that we need to shade."

I've rebased on top of master. The tests correctly failed, and swapping to underscores lets them now pass.

carl-mastrangelo · 2017-08-31T21:43:04Z

netty/shaded/src/testShadow/java/io/grpc/netty/shaded/ShadingTest.java

+  /** Test TLS since that verifies everything, including tcnative, is functional. */
+  @Test
+  public void tls() throws Exception {
+    Server server = ServerBuilder.forPort(0)


If you are testing TLS, can you for the SslProvider to be SslProvider.OPENSSL?

Also, this test case should probably be split. It is testing two things: If the ServerLoader worked, and if tcnative was shaded properly.

Done. Done.

carl-mastrangelo · 2017-08-31T21:43:45Z

netty/shaded/src/testShadow/java/io/grpc/netty/shaded/ShadingTest.java

+    } finally {
+      server.shutdownNow();
+    }
+  }


Await termination?

ejona86 · 2017-09-14T17:02:24Z

@carl-mastrangelo, PTAL

carl-mastrangelo

LGTM

carl-mastrangelo · 2017-09-14T21:33:48Z

netty/shaded/src/testShadow/java/io/grpc/netty/shaded/ShadingTest.java

-  public void tls() throws Exception {
-    Server server = ServerBuilder.forPort(0)
+  public void serviceLoaderFindsNetty() throws Exception {
+    assertTrue(ServerBuilder.forPort(0) instanceof NettyServerBuilder);


optional: if you use Truth.assertThat(ServerBuilder.forPort(0)).isInstanceOf(NettyServerBuilder.class), a test failure will tell you what the instance actually was.

I've swapped the tests to use Truth exclusively.

ejona86 · 2017-09-15T16:29:54Z

retest this please

ejona86 · 2017-09-15T17:39:19Z

The dll isn't being loaded on Windows, and it's a PITA to figure out why because java.util.logging is broken in unit tests. I'll need to set up a full dev environment on Windows to figure out what's wrong.

The entire transport is shaded because we wouldn't want to collide with the unshaded form of grpc-netty.

ejona86 · 2017-12-08T22:01:33Z

Soo... I set up an environment, and then I rebased, and saw everything working. I'm assuming an update to Netty fixed the problem.

ejona86 · 2017-12-08T22:03:44Z

It was almost less than a year 😄

garrettjonesgoogle · 2017-12-08T22:14:46Z

lol that's an interesting way to characterize "more than a year" :-P

mastergberry · 2018-03-16T17:08:33Z

Hey @ejona86 we are trying to use GRPC 1.10 in our program and are having some netty conflictions. Wasn't this issue/change in 1.9 supposed to make it so GRPC used their own netty and didn't conflict with other libraries that used netty too?

https://github.com/grpc/grpc-java/blob/master/SECURITY.md#troubleshooting

We were going over this and wanted to know if it was completely up to date...because GRPC is using the wrong netty version when trying to write a response in our current application and this is causing issues.

ejona86 · 2018-03-19T17:38:01Z

@mastergberry, as mentioned in the in the v1.9.0 release notes and my proposed changes to SECURITY.md, you have to swap to grpc-netty-shaded to get the pre-shaded Netty. If you use grpc-netty then you will see the original behavior.

mastergberry · 2018-03-19T19:15:27Z

My apologies. Thanks for the nudge in the right direction.

ejona86 mentioned this pull request Jan 27, 2017

Using Pub/Sub JAVA client in play framework 2.5 - netty conflicts googleapis/google-cloud-java#1331

Closed

igorbernstein2 mentioned this pull request Jun 9, 2017

Upgrading grpc to 1.4.0 googleapis/java-bigtable-hbase#1395

Merged

ejona86 force-pushed the shade-netty branch from 9f7cd0b to 753994c Compare July 8, 2017 06:15

ejona86 changed the title ~~RFC: Shade Netty transport~~ Shade Netty transport Jul 21, 2017

igorbernstein2 mentioned this pull request Aug 4, 2017

netty-tcnative conflict, and using hadoop googleapis/java-bigtable-hbase#1475

Closed

ejona86 mentioned this pull request Aug 11, 2017

<RequestT,ResponseT>create(...) in MethodDescriptor has been deprecated #3332

Closed

igorbernstein2 mentioned this pull request Aug 21, 2017

Shade tcnative & update grpc googleapis/java-bigtable-hbase#1487

Merged

ejona86 requested review from zhangkun83 and carl-mastrangelo August 29, 2017 02:08

carl-mastrangelo reviewed Aug 29, 2017

View reviewed changes

carl-mastrangelo reviewed Aug 31, 2017

View reviewed changes

zhangkun83 approved these changes Sep 1, 2017

View reviewed changes

carl-mastrangelo approved these changes Sep 14, 2017

View reviewed changes

ejona86 force-pushed the shade-netty branch 2 times, most recently from 9a18305 to 4f0e2f6 Compare September 15, 2017 16:24

igorbernstein2 mentioned this pull request Sep 19, 2017

Consider publishing a shaded version of bigtable-client-core. googleapis/java-bigtable-hbase#1500

Closed

ejona86 added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Dec 8, 2017

kokoro-team removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Dec 8, 2017

Shade Netty transport, with tcnative

274cbc3

The entire transport is shaded because we wouldn't want to collide with the unshaded form of grpc-netty.

ejona86 force-pushed the shade-netty branch from 4f0e2f6 to 274cbc3 Compare December 8, 2017 21:46

ejona86 merged commit 83d710b into grpc:master Dec 8, 2017

ejona86 deleted the shade-netty branch December 8, 2017 22:02

ejona86 added this to the 1.9 milestone Dec 8, 2017

ejona86 added the Type: Feature label Dec 8, 2017

patriknw mentioned this pull request Feb 9, 2018

Use grpc-netty-shaded to avoid conflict with other Netty version akka/akka-grpc#1

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

Shade Netty transport #2485

Shade Netty transport #2485

Conversation

ejona86 commented Dec 5, 2016 • edited Loading

jhspaybar commented Dec 8, 2016

ejona86 commented Dec 8, 2016

jhspaybar commented Dec 8, 2016

nevi-me commented Jan 9, 2017

ejona86 commented Jan 9, 2017

zackangelo commented Jan 12, 2017

ejona86 commented Jan 24, 2017

nevi-me commented Jan 24, 2017

ejona86 commented Jan 27, 2017

jhspaybar commented Feb 2, 2017

garrettjonesgoogle commented Feb 9, 2017

kuroneko25 commented Feb 9, 2017

ejona86 commented Mar 10, 2017

zackangelo commented Mar 21, 2017

jhspaybar commented May 3, 2017

nevi-me commented May 3, 2017

jhspaybar commented May 3, 2017

nevi-me commented May 3, 2017

ejona86 commented May 3, 2017

vkedia commented Jun 8, 2017

ejona86 commented Jul 8, 2017

ejona86 commented Jul 12, 2017

ejona86 commented Jul 21, 2017

ejona86 commented Aug 29, 2017

ejona86 commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejona86 commented Sep 14, 2017

carl-mastrangelo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejona86 Sep 15, 2017 • edited Loading

Choose a reason for hiding this comment

ejona86 commented Sep 15, 2017

ejona86 commented Sep 15, 2017

ejona86 commented Dec 8, 2017

ejona86 commented Dec 8, 2017

garrettjonesgoogle commented Dec 8, 2017

mastergberry commented Mar 16, 2018

ejona86 commented Mar 19, 2018 • edited Loading

mastergberry commented Mar 19, 2018

ejona86 commented Dec 5, 2016 •

edited

Loading

ejona86 Sep 15, 2017 •

edited

Loading

ejona86 commented Mar 19, 2018 •

edited

Loading