-
Notifications
You must be signed in to change notification settings - Fork 916
Bump Netty to 4.2.2 #6205
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
base: master
Are you sure you want to change the base?
Bump Netty to 4.2.2 #6205
Conversation
|
@@ -336,7 +336,6 @@ public static SslHandler newSslHandler(SslContext sslContext, ByteBufAllocator a | |||
*/ | |||
private static void configureSslEngine(SSLEngine sslEngine) { | |||
SSLParameters sslParameters = sslEngine.getSSLParameters(); | |||
sslParameters.setEndpointIdentificationAlgorithm("HTTPS"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer needed, defaults to HTTPS
in 4.2, previously was null
Covered in existing test
https://github.com/aws/aws-sdk-java-v2/blob/master/http-clients/netty-nio-client/src/test/java/software/amazon/awssdk/http/nio/netty/internal/utils/NettyUtilsTest.java#L86
@@ -0,0 +1,6 @@ | |||
{ | |||
"type": "feature", | |||
"category": "AWS SDK for Java v2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be "Netty NIO HTTP Client"
@@ -116,7 +116,7 @@ | |||
</dependency> | |||
<dependency> | |||
<groupId>io.netty</groupId> | |||
<artifactId>netty-codec</artifactId> | |||
<artifactId>netty-codec-base</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did Netty deprecate netty-codec
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The netty-codec module has been split into a number of different sub-modules, which the netty-codec module then depends on. In other words, netty-codec is now multiple jar files instead of one.
* If {@link MultiThreadIoEventLoopGroup} is passed in, {@link NioSocketChannel} and {@link NioDatagramChannel} will be | ||
* resolved, regardless of the transport {@link IoHandlerFactory} set on the {@link MultiThreadIoEventLoopGroup}. This is | ||
* because it is not possible to determine the type of transport factory from a given {@link MultiThreadIoEventLoopGroup}. | ||
* <p> | ||
* To use a {@link MultiThreadIoEventLoopGroup} with a non-Nio transport factory, use | ||
* {@link #create(EventLoopGroup, ChannelFactory, ChannelFactory)}, specifying the socket and datagram channels. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way this is worded is hard to follow. From what I understand, if the customer calls this with a MultiThreadIoEventLoopGroup
(and not one of its subclasses like NioEventLoopGroup
), then the SDK will disregard the IoHandler
the event loop group was constructed with when choosing the factory, and always assume NioSocketChannel::new
.
And if they want to ensure that specific factories are used, to use one of the other overloads?
What happens if there is a mismatch in the IoHandlerFactory
and the resolved SocketFactory
(e.g. epoll vs NIO)? Is there a clear error message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if they want to ensure that specific factories are used, to use one of the other overloads?
Correct, I'll try to make the wording more clear
What happens if there is a mismatch in the IoHandlerFactory and the resolved SocketFactory (e.g. epoll vs NIO)? Is there a clear error message?
Tested this:
SdkEventLoopGroup sdkEventLoopGroup =
SdkEventLoopGroup.create(new MultiThreadIoEventLoopGroup(KQueueIoHandler.newFactory()),
NioSocketChannel::new,
NioDatagramChannel::new);
SdkAsyncHttpClient netty = NettyNioAsyncHttpClient.builder()
.eventLoopGroup(sdkEventLoopGroup)
.build();
S3AsyncClient asyncClient = S3AsyncClient.builder()
.region(DEFAULT_REGION)
.credentialsProvider(CREDENTIALS_PROVIDER_CHAIN)
.httpClient(netty)
.build();
asyncClient.headObject(c -> c.bucket(BUCKET).key(ASYNC_KEY)).join();
Got error message:
java.util.concurrent.CompletionException: software.amazon.awssdk.core.exception.SdkClientException: Unable to execute HTTP request: incompatible event loop type: io.netty.channel.SingleThreadIoEventLoop (SDK Attempt Count: 1)
...
Caused by: java.lang.IllegalStateException: incompatible event loop type: io.netty.channel.SingleThreadIoEventLoop
at io.netty.channel.AbstractChannel$AbstractUnsafe.register(AbstractChannel.java:332)
at io.netty.channel.SingleThreadEventLoop.register(SingleThreadEventLoop.java:119)
at io.netty.channel.SingleThreadEventLoop.register(SingleThreadEventLoop.java:113)
at io.netty.channel.MultithreadEventLoopGroup.register(MultithreadEventLoopGroup.java:86)
at software.amazon.awssdk.http.nio.netty.internal.DelegatingEventLoopGroup.register(DelegatingEventLoopGroup.java:173)
at io.netty.bootstrap.AbstractBootstrap.initAndRegister(AbstractBootstrap.java:339)
at io.netty.bootstrap.Bootstrap.doResolveAndConnect(Bootstrap.java:164)
at io.netty.bootstrap.Bootstrap.connect(Bootstrap.java:125)
|
Motivation and Context
Bump Netty to 4.2.2 Final
https://github.com/netty/netty/wiki/Netty-4.2-Migration-Guide
Modifications
netty-codec
withnetty-codec-base
NioEventLoopGroup
withMultiThreadIoEventLoopGroup
netty-open-ssl-version
to2.0.72.Final
bcpkix-jdk15on
withbcpkix-jdk18on
and bump to1.80
SdkEventLoopGroup.create()
to allow passing in socket and datagram channelsTesting
Integ tests passed
Ran
sdk-benchmarks
for Netty with both4.1.118.Final
and4.2.2.Final
4.1.118
4.2.2
With the version bump to 4.2.2:
For concurrent calls, we see 12% improvement for both JDK and OpenSSL providers.
For sequential calls, we see 7% drop for JDK provider, and 2% drop for OpenSSL provider
Screenshots (if appropriate)
Types of changes
Checklist
mvn install
succeedsscripts/new-change
script and following the instructions. Commit the new file created by the script in.changes/next-release
with your changes.License