Apache9 commented on code in PR #4125:
URL: https://github.com/apache/hbase/pull/4125#discussion_r930850102
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcConnection.java:
##########
@@ -112,49 +110,62 @@ class NettyRpcConnection extends RpcConnection {
header.writeTo(new ByteBufOutputStream(this.connectionHeaderWithLength));
}
+ private Channel getChannel() {
+ return channel;
+ }
+
@Override
protected void callTimeout(Call call) {
- execute(eventLoop, () -> {
- if (channel != null) {
- channel.pipeline().fireUserEventTriggered(new CallEvent(TIMEOUT,
call));
- }
- });
+ Channel channel = getChannel();
Review Comment:
Why this change?
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcConnection.java:
##########
@@ -66,12 +75,6 @@
/**
* RPC connection implementation based on netty.
- * <p/>
Review Comment:
Why removing these comments?
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcConnection.java:
##########
@@ -112,49 +110,62 @@ class NettyRpcConnection extends RpcConnection {
header.writeTo(new ByteBufOutputStream(this.connectionHeaderWithLength));
}
+ private Channel getChannel() {
+ return channel;
+ }
+
@Override
protected void callTimeout(Call call) {
- execute(eventLoop, () -> {
- if (channel != null) {
- channel.pipeline().fireUserEventTriggered(new CallEvent(TIMEOUT,
call));
- }
- });
+ Channel channel = getChannel();
+
+ if (channel != null) {
+ channel.pipeline().fireUserEventTriggered(new CallEvent(TIMEOUT, call));
+ }
}
@Override
public boolean isActive() {
- return channel != null;
+ return getChannel() != null;
}
- private void shutdown0() {
- assert eventLoop.inEventLoop();
- if (channel != null) {
- channel.close();
+ @Override
+ public void shutdown() {
+ ChannelFuture currentChannelFuture;
+ Channel currentChannel;
+
+ synchronized (connectLock) {
+ currentChannelFuture = channelFuture;
+ currentChannel = channel;
+ channelFuture = null;
channel = null;
}
- }
- @Override
- public void shutdown() {
- execute(eventLoop, this::shutdown0);
+ if (currentChannelFuture == null) {
+ return;
+ }
+
+ if (!currentChannelFuture.isDone()) {
+ currentChannelFuture.cancel(true);
+ }
+
+ if (currentChannel != null) {
+ currentChannel.close();
+ }
}
@Override
public void cleanupConnection() {
- execute(eventLoop, () -> {
- if (connectionHeaderPreamble != null) {
- ReferenceCountUtil.safeRelease(connectionHeaderPreamble);
- connectionHeaderPreamble = null;
- }
- if (connectionHeaderWithLength != null) {
- ReferenceCountUtil.safeRelease(connectionHeaderWithLength);
- connectionHeaderWithLength = null;
- }
- });
+ if (connectionHeaderPreamble != null) {
+ ReferenceCountUtil.safeRelease(connectionHeaderPreamble);
+ connectionHeaderPreamble = null;
+ }
+ if (connectionHeaderWithLength != null) {
+ ReferenceCountUtil.safeRelease(connectionHeaderWithLength);
+ connectionHeaderWithLength = null;
+ }
}
- private void established(Channel ch) throws IOException {
- assert eventLoop.inEventLoop();
Review Comment:
Why removing this assert? I'm totally confused why we need to change so much
assumptions on the implementation of this class? Is it really necessary?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
