[FLINK-36876] Support external eventLoopGroup for RestClient #25788
Conversation
| ConcurrentHashMap.newKeySet(); | ||
|
|
||
| private final List<OutboundChannelHandlerFactory> outboundChannelHandlerFactories; | ||
| private final Boolean useInternalEventLoopGroup; |
There was a problem hiding this comment.
nit: why not boolean, I assume we do not want this ever to be null
| WaitStrategy waitStrategy, | ||
| ClientHighAvailabilityServicesFactory clientHAServicesFactory) | ||
| ClientHighAvailabilityServicesFactory clientHAServicesFactory, | ||
| EventLoopGroup group) |
| selectStrategyFactory); | ||
| useInternalEventLoopGroup = true; | ||
| } else { | ||
| useInternalEventLoopGroup = false; |
There was a problem hiding this comment.
should we check that the supplied group is not shutting down or shutdown
There was a problem hiding this comment.
I see the channel is NioSocketChannel.class is this always the case for external groups?
There was a problem hiding this comment.
I think the extenal service doesn't care the type of SocketChannel. As the jira says, it just pass a shared event group to avoid heap memory leak.
Maybe it's a better choice to specify the contruct param group as NioEventLoopGroup, which could avoid the type error building Bootstrap instance ?
There was a problem hiding this comment.
should we check that the supplied group is not shutting down or shutdown
This check is necessary and the logic has been added. Thanks
|
Reviewed by Chi on 12/12/24. Asked submitter questions |
The code has been updated and the question has been answered. Please review again. |
|
This PR is being marked as stale since it has not had any activity in the last 90 days. If you are having difficulty finding a reviewer, please reach out to the If this PR is no longer valid or desired, please feel free to close it. |
|
@chenyuzhi459 can you please rebase on the latest master to retrigger the tests before we merge this? |
4 participants
What is the purpose of the change
Support external eventLoopGroup for RestClient
Brief change log
Verifying this change
The change is tested by RestClientTest.testExternalEventGroup
Does this pull request potentially affect one of the following parts:
@Public(Evolving): ( no)Documentation