-
Notifications
You must be signed in to change notification settings - Fork 26.4k
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
Use ExecutorService instead of event loop for Netty connection #13904
Use ExecutorService instead of event loop for Netty connection #13904
Conversation
This change addresses the issue of the event loop being blocked for an extended period, improving overall performance and responsiveness.
Ok. After fixing failed case, I'll make this ready. |
}, | ||
1L, | ||
TimeUnit.SECONDS); | ||
executor.submit(() -> { |
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.
Also, please create a new isolated executor to reconnect
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.
Thank you very much.
I resolved the error by defining the ExecutorService within the method. However, a new error has occurred, so I will check it.
Considering the current modification, instantiating it inside the method might be costly. Would it be better to have an instance of the ExecutorRepository class as a field in the NettyConnectionClient class?
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.
Yes, you're right. We need to maintain this executor inExecutorRepository
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 3.2 #13904 +/- ##
==========================================
+ Coverage 70.39% 70.42% +0.03%
==========================================
Files 1607 1607
Lines 70070 70094 +24
Branches 10098 10102 +4
==========================================
+ Hits 49327 49367 +40
+ Misses 16116 16084 -32
- Partials 4627 4643 +16 ☔ View full report in Codecov by Sentry. |
@@ -90,6 +95,8 @@ protected void initConnectionClient() { | |||
this.closePromise = new DefaultPromise<>(GlobalEventExecutor.INSTANCE); | |||
this.init = new AtomicBoolean(false); | |||
this.increase(); | |||
this.isReconnecting = new AtomicBoolean(false); | |||
scheduledExecutorService = Executors.newSingleThreadScheduledExecutor(); |
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.
This executor should be managed by org.apache.dubbo.common.threadpool.manager.FrameworkExecutorRepository
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.
Thank you for the suggestion. I will make the necessary modifications.
@icodening @AlbumenJ |
@@ -374,7 +385,7 @@ public void operationComplete(ChannelFuture future) { | |||
"Failed to connect to server: " + getConnectAddress()); | |||
} | |||
}, | |||
1L, | |||
1, |
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.
Please sync with org.apache.dubbo.remoting.exchange.support.header.HeaderExchangeClient#calculateReconnectDuration
to get the duration
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.
@AlbumenJ
Thank you for your review.
I have modified the code to get the idle time before reconnection. By default, it will now wait for 60000ms.
public AbstractClient(URL url, ChannelHandler handler) throws RemotingException { | ||
super(url, handler); | ||
// set default needReconnect true when channel is not connected | ||
needReconnect = url.getParameter(Constants.SEND_RECONNECT_KEY, true); | ||
|
||
applicationModel = url.getOrDefaultApplicationModel(); |
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.
Directly get Framework Model here would be better
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.
@AlbumenJ
Thank you for your review.
I've made the adjustments based on your suggestions. Could you please check if it aligns with your expectations?
@@ -138,6 +138,7 @@ void connectSyncTest() throws Throwable { | |||
|
|||
nettyPortUnificationServer.bind(); | |||
// auto reconnect | |||
Thread.sleep(60000); |
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.
Use awaitility to verify
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.
I've made the adjustments that use awaitility.
use awaitility
|
@icodening PTAL |
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.
LGTM
@EarthChen PTAL |
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.
LGTM
…e#13904) * Use ExecutorService instead of event loop for Netty connection This change addresses the issue of the event loop being blocked for an extended period, improving overall performance and responsiveness. * fix log getConnectAddress * applay format * applay format * applay format * applay format * applay format * Use an independent ExecutorService * Use ScheduledExecutorService for scheduling tasks * delete unnecessary files * Modify to stop ExecutorService using shutdownNow * Modify to use ScheduledExecutor managed by FrameworkExecutorRepository * Synchronize reconnectDuration with HeaderExchangeClient's reconnectDuration * get framework model directly use awaitility
This change addresses the issue of the event loop being blocked for an extended period, improving overall performance and responsiveness.
What is the purpose of the change
This PR addresses the issue #13853 where
connectionClient.doConnect()
was blocking the event loop for up to 3 seconds.To resolve this, the connection process has been moved to a separate thread using an
ExecutorService
.Brief changelog
Changes:
connectionClient.doConnect()
on the event loop.connectionClient.doConnect()
to anExecutorService
to execute in a separate thread.Verifying this change
Checklist