Skip to content

RATIS-1618. Resolve the stream client concurrent reconnection problem#676

Merged
szetszwo merged 3 commits intoapache:masterfrom
guohao-rosicky:guohao-RATIS-1618-dev
Jul 14, 2022
Merged

RATIS-1618. Resolve the stream client concurrent reconnection problem#676
szetszwo merged 3 commits intoapache:masterfrom
guohao-rosicky:guohao-RATIS-1618-dev

Conversation

@guohao-rosicky
Copy link
Contributor

What changes were proposed in this pull request?

When there is a network problem, reconnection is triggered.
If sending a lot of messages at the same time, multiple reconnections will be triggered.
In this case, we need to wait for the result of the first reconnection synchronously

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/RATIS-1618

@guohao-rosicky
Copy link
Contributor Author

What do you think of this change @szetszwo

Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guohao-rosicky , the change looks good. Just two minor comments inlined.

Comment on lines +229 to +230
} else {
workerGroup.shutdownGracefully();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The else is not needed since previous == null means already closed.

if (channelFuture != null) {
Channel channel = channelFuture.syncUninterruptibly().channel();
if (channel.isActive()) {
LOG.info("{} is already connect use exists channel {}", this, channel);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove LOG.info since this is a client code.

@guohao-rosicky
Copy link
Contributor Author

Hi, @szetszwo. Thanks for review my PR.
The modification is complete.

@guohao-rosicky guohao-rosicky requested a review from szetszwo July 14, 2022 03:41
Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 the change looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants