api,netty: fix MethodDescriptor and InternalKnownTransport for netty-shaded #6774
Conversation
| new InternalMethodDescriptor(InternalKnownTransport.NETTY); | ||
| new InternalMethodDescriptor( | ||
| NettyClientTransport.class.getName().contains("grpc.netty.shaded") | ||
| ? InternalKnownTransport.NETTY_SHADED : InternalKnownTransport.NETTY); |
There was a problem hiding this comment.
I'm not sure if it's possible that the compiler can optimize this contains()?: expression and replace it with InternalKnownTransport.NETTY directly before shadow plugin does its job. My local interop test did not show it happened.
There was a problem hiding this comment.
No, it won't be able to rework this string. You could also just do "shaded" as another option. But all those variations should be fine.
There was a problem hiding this comment.
Just "shaded" will produce false positive if grpc as a whole is shaded by the application.
|
Resolves #6765 |
| @@ -51,7 +51,8 @@ | |||
|
|
|||
| // Must be set to InternalKnownTransport.values().length | |||
| // Not referenced to break the dependency. | |||
| private final AtomicReferenceArray<Object> rawMethodNames = new AtomicReferenceArray<>(1); | |||
| private final AtomicReferenceArray<Object> rawMethodNames = | |||
| new AtomicReferenceArray<>(InternalKnownTransport.values().length); | |||
There was a problem hiding this comment.
This will break Bazel; use a literal 2.
The comment alludes to why this was done, but even if you thought that no longer applied the comment should have been removed.
There was a problem hiding this comment.
InternalKnownTransport is part of grpc-api so Bazel seems not impacted. It breaks blaze though.
There was a problem hiding this comment.
Just noticed there were already comments on this field
// Must be set to InternalKnownTransport.values().length
// Not referenced to break the dependency.
private final AtomicReferenceArray<Object> rawMethodNames =| new InternalMethodDescriptor(InternalKnownTransport.NETTY); | ||
| new InternalMethodDescriptor( | ||
| NettyClientTransport.class.getName().contains("grpc.netty.shaded") | ||
| ? InternalKnownTransport.NETTY_SHADED : InternalKnownTransport.NETTY); |
There was a problem hiding this comment.
No, it won't be able to rework this string. You could also just do "shaded" as another option. But all those variations should be fine.
ejona86
added
the
TODO:backport
ejona86
removed
the
TODO:backport
Continue to use enum
InternalKnownTransportand extend it for ABI backward compatibility (hopefully).