You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by "hanpen24 (via GitHub)" <gi...@apache.org> on 2024/03/11 12:43:36 UTC
[PR] Use ExecutorService instead of event loop for Netty connection [dubbo]
hanpen24 opened a new pull request, #13904:
URL: https://github.com/apache/dubbo/pull/13904
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:
- Removed the scheduling of `connectionClient.doConnect()` on the event loop.
- Added submission of `connectionClient.doConnect()` to an `ExecutorService` to execute in a separate thread.
## Verifying this change
<!-- Follow this checklist to help us incorporate your contribution quickly and easily: -->
## Checklist
- [x] Make sure there is a [GitHub_issue](https://github.com/apache/dubbo/issues) field for the change (usually before you start working on it). Trivial changes like typos do not require a GitHub issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
- [ ] Each commit in the pull request should have a meaningful subject line and body.
- [ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [ ] Check if is necessary to patch to Dubbo 3 if you are work on Dubbo 2.7
- [ ] Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add sample in [dubbo samples](https://github.com/apache/dubbo-samples) project.
- [ ] Add some description to [dubbo-website](https://github.com/apache/dubbo-website) project if you are requesting to add a feature.
- [ ] GitHub Actions works fine on your own branch.
- [ ] If this contribution is large, please follow the [Software Donation Guide](https://github.com/apache/dubbo/wiki/Software-donation-guide).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Use ExecutorService instead of event loop for Netty connection [dubbo]
Posted by "EarthChen (via GitHub)" <gi...@apache.org>.
EarthChen merged PR #13904:
URL: https://github.com/apache/dubbo/pull/13904
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Use ExecutorService instead of event loop for Netty connection [dubbo]
Posted by "hanpen24 (via GitHub)" <gi...@apache.org>.
hanpen24 commented on PR #13904:
URL: https://github.com/apache/dubbo/pull/13904#issuecomment-2006652432
@icodening @AlbumenJ
I have made the revisions. Could you please review them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Use ExecutorService instead of event loop for Netty connection [dubbo]
Posted by "AlbumenJ (via GitHub)" <gi...@apache.org>.
AlbumenJ commented on PR #13904:
URL: https://github.com/apache/dubbo/pull/13904#issuecomment-1991184826
Please fix failed case
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Use ExecutorService instead of event loop for Netty connection [dubbo]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #13904:
URL: https://github.com/apache/dubbo/pull/13904#issuecomment-2005552240
## [](https://sonarcloud.io/dashboard?id=apache_dubbo&pullRequest=13904) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo&pullRequest=13904&resolved=false&inNewCodePeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_dubbo&pullRequest=13904&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo&pullRequest=13904&resolved=false&inNewCodePeriod=true)
 [37.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo&pullRequest=13904&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo&pullRequest=13904&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_dubbo&pullRequest=13904)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Use ExecutorService instead of event loop for Netty connection [dubbo]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #13904:
URL: https://github.com/apache/dubbo/pull/13904#issuecomment-2002453203
## [Codecov](https://app.codecov.io/gh/apache/dubbo/pull/13904?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 70.48%. Comparing base [(`2f6b4c0`)](https://app.codecov.io/gh/apache/dubbo/commit/2f6b4c0c436b34741780ff4c7bd934e1e9e743a5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`99f354f`)](https://app.codecov.io/gh/apache/dubbo/pull/13904?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 11 commits behind head on 3.2.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 3.2 #13904 +/- ##
==========================================
+ Coverage 70.39% 70.48% +0.09%
==========================================
Files 1607 1607
Lines 70070 70077 +7
Branches 10098 10102 +4
==========================================
+ Hits 49327 49397 +70
+ Misses 16116 16054 -62
+ Partials 4627 4626 -1
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/dubbo/pull/13904?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Use ExecutorService instead of event loop for Netty connection [dubbo]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #13904:
URL: https://github.com/apache/dubbo/pull/13904#issuecomment-1994717610
## [](https://sonarcloud.io/dashboard?id=apache_dubbo&pullRequest=13904) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_dubbo&pullRequest=13904&resolved=false&inNewCodePeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_dubbo&pullRequest=13904&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo&pullRequest=13904&resolved=false&inNewCodePeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo&pullRequest=13904&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo&pullRequest=13904&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_dubbo&pullRequest=13904)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Use ExecutorService instead of event loop for Netty connection [dubbo]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #13904:
URL: https://github.com/apache/dubbo/pull/13904#issuecomment-2002451964
## [](https://sonarcloud.io/dashboard?id=apache_dubbo&pullRequest=13904) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_dubbo&pullRequest=13904&resolved=false&inNewCodePeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_dubbo&pullRequest=13904&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo&pullRequest=13904&resolved=false&inNewCodePeriod=true)
 [66.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo&pullRequest=13904&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo&pullRequest=13904&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_dubbo&pullRequest=13904)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Use ExecutorService instead of event loop for Netty connection [dubbo]
Posted by "hanpen24 (via GitHub)" <gi...@apache.org>.
hanpen24 commented on code in PR #13904:
URL: https://github.com/apache/dubbo/pull/13904#discussion_r1529493990
##########
dubbo-remoting/dubbo-remoting-netty4/src/main/java/org/apache/dubbo/remoting/transport/netty4/NettyConnectionClient.java:
##########
@@ -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();
Review Comment:
Thank you for the suggestion. I will make the necessary modifications.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Use ExecutorService instead of event loop for Netty connection [dubbo]
Posted by "hanpen24 (via GitHub)" <gi...@apache.org>.
hanpen24 commented on code in PR #13904:
URL: https://github.com/apache/dubbo/pull/13904#discussion_r1535420951
##########
dubbo-remoting/dubbo-remoting-api/src/main/java/org/apache/dubbo/remoting/transport/AbstractClient.java:
##########
@@ -55,13 +63,23 @@ public abstract class AbstractClient extends AbstractEndpoint implements Client
protected volatile ExecutorService executor;
+ protected volatile ScheduledExecutorService connectivityExecutor;
+
+ private ApplicationModel applicationModel;
+
+ protected long reconnectDuaration;
+
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();
Review Comment:
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Use ExecutorService instead of event loop for Netty connection [dubbo]
Posted by "hanpen24 (via GitHub)" <gi...@apache.org>.
hanpen24 commented on code in PR #13904:
URL: https://github.com/apache/dubbo/pull/13904#discussion_r1534167182
##########
dubbo-remoting/dubbo-remoting-netty4/src/main/java/org/apache/dubbo/remoting/transport/netty4/NettyConnectionClient.java:
##########
@@ -374,7 +385,7 @@ public void operationComplete(ChannelFuture future) {
"Failed to connect to server: " + getConnectAddress());
}
},
- 1L,
+ 1,
Review Comment:
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Use ExecutorService instead of event loop for Netty connection [dubbo]
Posted by "hanpen24 (via GitHub)" <gi...@apache.org>.
hanpen24 commented on code in PR #13904:
URL: https://github.com/apache/dubbo/pull/13904#discussion_r1523487584
##########
dubbo-remoting/dubbo-remoting-netty4/src/main/java/org/apache/dubbo/remoting/transport/netty4/NettyConnectionClient.java:
##########
@@ -361,21 +360,14 @@ public void operationComplete(ChannelFuture future) {
"%s is reconnecting, attempt=%d cause=%s",
connectionClient, 0, future.cause().getMessage()));
}
- final EventLoop loop = future.channel().eventLoop();
- loop.schedule(
- () -> {
- try {
- connectionClient.doConnect();
- } catch (RemotingException e) {
- LOGGER.error(
- TRANSPORT_FAILED_RECONNECT,
- "",
- "",
- "Failed to connect to server: " + getConnectAddress());
- }
- },
- 1L,
- TimeUnit.SECONDS);
+ executor.submit(() -> {
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Use ExecutorService instead of event loop for Netty connection [dubbo]
Posted by "AlbumenJ (via GitHub)" <gi...@apache.org>.
AlbumenJ commented on code in PR #13904:
URL: https://github.com/apache/dubbo/pull/13904#discussion_r1531444349
##########
dubbo-remoting/dubbo-remoting-netty4/src/main/java/org/apache/dubbo/remoting/transport/netty4/NettyConnectionClient.java:
##########
@@ -374,7 +385,7 @@ public void operationComplete(ChannelFuture future) {
"Failed to connect to server: " + getConnectAddress());
}
},
- 1L,
+ 1,
Review Comment:
Please sync with `org.apache.dubbo.remoting.exchange.support.header.HeaderExchangeClient#calculateReconnectDuration` to get the duration
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Use ExecutorService instead of event loop for Netty connection [dubbo]
Posted by "AlbumenJ (via GitHub)" <gi...@apache.org>.
AlbumenJ commented on code in PR #13904:
URL: https://github.com/apache/dubbo/pull/13904#discussion_r1534959792
##########
dubbo-remoting/dubbo-remoting-netty4/src/test/java/org/apache/dubbo/remoting/transport/netty4/ConnectionTest.java:
##########
@@ -138,6 +138,7 @@ void connectSyncTest() throws Throwable {
nettyPortUnificationServer.bind();
// auto reconnect
+ Thread.sleep(60000);
Review Comment:
Use awaitility to verify
##########
dubbo-remoting/dubbo-remoting-api/src/main/java/org/apache/dubbo/remoting/transport/AbstractClient.java:
##########
@@ -55,13 +63,23 @@ public abstract class AbstractClient extends AbstractEndpoint implements Client
protected volatile ExecutorService executor;
+ protected volatile ScheduledExecutorService connectivityExecutor;
+
+ private ApplicationModel applicationModel;
+
+ protected long reconnectDuaration;
+
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();
Review Comment:
Directly get Framework Model here would be better
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Use ExecutorService instead of event loop for Netty connection [dubbo]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #13904:
URL: https://github.com/apache/dubbo/pull/13904#issuecomment-2014836903
## [](https://sonarcloud.io/dashboard?id=apache_dubbo&pullRequest=13904) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_dubbo&pullRequest=13904&resolved=false&inNewCodePeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_dubbo&pullRequest=13904&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo&pullRequest=13904&resolved=false&inNewCodePeriod=true)
 [25.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo&pullRequest=13904&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo&pullRequest=13904&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_dubbo&pullRequest=13904)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Use ExecutorService instead of event loop for Netty connection [dubbo]
Posted by "hanpen24 (via GitHub)" <gi...@apache.org>.
hanpen24 commented on code in PR #13904:
URL: https://github.com/apache/dubbo/pull/13904#discussion_r1535422501
##########
dubbo-remoting/dubbo-remoting-netty4/src/test/java/org/apache/dubbo/remoting/transport/netty4/ConnectionTest.java:
##########
@@ -138,6 +138,7 @@ void connectSyncTest() throws Throwable {
nettyPortUnificationServer.bind();
// auto reconnect
+ Thread.sleep(60000);
Review Comment:
I've made the adjustments that use awaitility.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Use ExecutorService instead of event loop for Netty connection [dubbo]
Posted by "AlbumenJ (via GitHub)" <gi...@apache.org>.
AlbumenJ commented on PR #13904:
URL: https://github.com/apache/dubbo/pull/13904#issuecomment-2020123712
@icodening PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Use ExecutorService instead of event loop for Netty connection [dubbo]
Posted by "AlbumenJ (via GitHub)" <gi...@apache.org>.
AlbumenJ commented on code in PR #13904:
URL: https://github.com/apache/dubbo/pull/13904#discussion_r1524312836
##########
dubbo-remoting/dubbo-remoting-netty4/src/main/java/org/apache/dubbo/remoting/transport/netty4/NettyConnectionClient.java:
##########
@@ -361,21 +360,14 @@ public void operationComplete(ChannelFuture future) {
"%s is reconnecting, attempt=%d cause=%s",
connectionClient, 0, future.cause().getMessage()));
}
- final EventLoop loop = future.channel().eventLoop();
- loop.schedule(
- () -> {
- try {
- connectionClient.doConnect();
- } catch (RemotingException e) {
- LOGGER.error(
- TRANSPORT_FAILED_RECONNECT,
- "",
- "",
- "Failed to connect to server: " + getConnectAddress());
- }
- },
- 1L,
- TimeUnit.SECONDS);
+ executor.submit(() -> {
Review Comment:
Yes, you're right. We need to maintain this executor in`ExecutorRepository`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Use ExecutorService instead of event loop for Netty connection [dubbo]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #13904:
URL: https://github.com/apache/dubbo/pull/13904#issuecomment-2002425082
## [](https://sonarcloud.io/dashboard?id=apache_dubbo&pullRequest=13904) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_dubbo&pullRequest=13904&resolved=false&inNewCodePeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_dubbo&pullRequest=13904&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo&pullRequest=13904&resolved=false&inNewCodePeriod=true)
 [77.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo&pullRequest=13904&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo&pullRequest=13904&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_dubbo&pullRequest=13904)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Use ExecutorService instead of event loop for Netty connection [dubbo]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #13904:
URL: https://github.com/apache/dubbo/pull/13904#issuecomment-2012661936
## [](https://sonarcloud.io/dashboard?id=apache_dubbo&pullRequest=13904) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo&pullRequest=13904&resolved=false&inNewCodePeriod=true)
 [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_dubbo&pullRequest=13904&metric=new_accepted_issues&view=list)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo&pullRequest=13904&resolved=false&inNewCodePeriod=true)
 [25.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo&pullRequest=13904&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo&pullRequest=13904&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_dubbo&pullRequest=13904)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Use ExecutorService instead of event loop for Netty connection [dubbo]
Posted by "AlbumenJ (via GitHub)" <gi...@apache.org>.
AlbumenJ commented on code in PR #13904:
URL: https://github.com/apache/dubbo/pull/13904#discussion_r1521404933
##########
dubbo-remoting/dubbo-remoting-netty4/src/main/java/org/apache/dubbo/remoting/transport/netty4/NettyConnectionClient.java:
##########
@@ -361,21 +360,14 @@ public void operationComplete(ChannelFuture future) {
"%s is reconnecting, attempt=%d cause=%s",
connectionClient, 0, future.cause().getMessage()));
}
- final EventLoop loop = future.channel().eventLoop();
- loop.schedule(
- () -> {
- try {
- connectionClient.doConnect();
- } catch (RemotingException e) {
- LOGGER.error(
- TRANSPORT_FAILED_RECONNECT,
- "",
- "",
- "Failed to connect to server: " + getConnectAddress());
- }
- },
- 1L,
- TimeUnit.SECONDS);
+ executor.submit(() -> {
Review Comment:
Also, please create a new isolated executor to reconnect
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Use ExecutorService instead of event loop for Netty connection [dubbo]
Posted by "hanpen24 (via GitHub)" <gi...@apache.org>.
hanpen24 commented on PR #13904:
URL: https://github.com/apache/dubbo/pull/13904#issuecomment-1991246005
Ok. After fixing failed case, I'll make this ready.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Use ExecutorService instead of event loop for Netty connection [dubbo]
Posted by "AlbumenJ (via GitHub)" <gi...@apache.org>.
AlbumenJ commented on code in PR #13904:
URL: https://github.com/apache/dubbo/pull/13904#discussion_r1527703947
##########
dubbo-remoting/dubbo-remoting-netty4/src/main/java/org/apache/dubbo/remoting/transport/netty4/NettyConnectionClient.java:
##########
@@ -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();
Review Comment:
This executor should be managed by `org.apache.dubbo.common.threadpool.manager.FrameworkExecutorRepository`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] Use ExecutorService instead of event loop for Netty connection [dubbo]
Posted by "AlbumenJ (via GitHub)" <gi...@apache.org>.
AlbumenJ commented on PR #13904:
URL: https://github.com/apache/dubbo/pull/13904#issuecomment-2026547359
@EarthChen PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org