-
Notifications
You must be signed in to change notification settings - Fork 26.6k
Fix mistaken deletion of reconnect interval #15613
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
Conversation
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.
Pull Request Overview
This PR fixes a mistaken deletion of the reconnect interval by restoring scheduled reconnection logic with proper timing control.
- Replaces immediate
doReconnect()call with scheduleddoConnect()execution - Adds error handling and logging for connection failures during reconnection
- Restores the use of
reconnectDurationfor controlling reconnection timing
| connectivityExecutor.schedule( | ||
| () -> { | ||
| try { | ||
| doConnect(); |
Copilot
AI
Aug 5, 2025
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 change from doReconnect() to doConnect() may alter the reconnection behavior. doReconnect() likely contains reconnection-specific logic that differs from the initial connection logic in doConnect(). Consider verifying that doConnect() handles reconnection scenarios appropriately or restore the use of doReconnect().
| doConnect(); | |
| doReconnect(); |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 3.3 #15613 +/- ##
============================================
- Coverage 61.06% 61.03% -0.03%
+ Complexity 11693 12 -11681
============================================
Files 1909 1909
Lines 86783 86781 -2
Branches 13094 13095 +1
============================================
- Hits 52994 52967 -27
- Misses 28373 28395 +22
- Partials 5416 5419 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
finefuture
left a comment
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 recommend making this change:
protected void scheduleReconnect(long reconnectDuration, TimeUnit unit) {
connectivityExecutor.schedule(() -> {
try {
doConnect();
} catch (RemotingException e) {
logger.error(
TRANSPORT_FAILED_RECONNECT, "", "", "Failed to reconnect to server: " + getConnectAddress());
}
}, reconnectDuration, unit);
}
in ConnectionListener
public void operationComplete(ChannelFuture future) {
if (!isReconnecting.compareAndSet(true, false)) {
return;
}
if (future.isSuccess()) {
return;
}
AbstractNettyConnectionClient connectionClient = AbstractNettyConnectionClient.this;
if (connectionClient.isClosed() || connectionClient.getCounter() == 0) {
if (logger.isDebugEnabled()) {
logger.debug(
"Connection:{} aborted to reconnect. {}",
connectionClient,
future.cause().getMessage());
}
return;
}
if (logger.isDebugEnabled()) {
logger.debug(
"Connection:{} is reconnecting, attempt=0 cause={}",
connectionClient,
future.cause().getMessage());
}
// Notify the connection is unavailable.
disconnectedPromise.trySuccess(null);
scheduleReconnect(reconnectDuration, TimeUnit.MILLISECONDS);
}
in NettyConnectionHandler
public void reconnect(Object channel) {
if (!(channel instanceof Channel)) {
return;
}
Channel nettyChannel = ((Channel) channel);
if (LOGGER.isDebugEnabled()) {
LOGGER.debug("Connection:{} is reconnecting, attempt={}", connectionClient, 1);
}
if (connectionClient.isClosed()) {
LOGGER.info("The connection {} has been closed and will not reconnect", connectionClient);
return;
}
connectionClient.scheduleReconnect(1, TimeUnit.SECONDS);
}
finefuture
left a comment
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.
It would be great if we could delete this useless variable.
in org.apache.dubbo.remoting.transport.netty4.NettyConnectionHandler#reconnect
EventLoop eventLoop = nettyChannel.eventLoop();
finefuture
left a comment
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.
What is the purpose of the change?
Fix mistaken deletion of reconnect interval