Fix mistaken deletion of reconnect interval#15613
Merged
zrlw merged 1 commit intoapache:3.3from Aug 6, 2025
Merged
Conversation
Contributor
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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().
Suggested change
| 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
reviewed
Aug 5, 2025
Contributor
finefuture
left a comment
finefuture
left a comment
There was a problem hiding this comment.
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
requested changes
Aug 5, 2025
Contributor
finefuture
left a comment
finefuture
left a comment
There was a problem hiding this comment.
It would be great if we could delete this useless variable.
in org.apache.dubbo.remoting.transport.netty4.NettyConnectionHandler#reconnect
EventLoop eventLoop = nettyChannel.eventLoop();
zrlw
approved these changes
Aug 6, 2025
MoritzArena
pushed a commit
to MoritzArena/dubbo
that referenced
this pull request
Sep 4, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the purpose of the change?
Fix mistaken deletion of reconnect interval