You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ratis.apache.org by GitBox <gi...@apache.org> on 2020/10/28 18:19:28 UTC
[GitHub] [incubator-ratis] szetszwo opened a new pull request #236: RATIS-1114. Change NettyServerStreamRpc to use only one ConcurrentMap.
szetszwo opened a new pull request #236:
URL: https://github.com/apache/incubator-ratis/pull/236
See https://issues.apache.org/jira/browse/RATIS-1114
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang merged pull request #236: RATIS-1114. Change NettyServerStreamRpc to use only one ConcurrentMap.
Posted by GitBox <gi...@apache.org>.
runzhiwang merged pull request #236:
URL: https://github.com/apache/incubator-ratis/pull/236
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang commented on pull request #236: RATIS-1114. Change NettyServerStreamRpc to use only one ConcurrentMap.
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #236:
URL: https://github.com/apache/incubator-ratis/pull/236#issuecomment-718274804
LGTM
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] amaliujia commented on a change in pull request #236: RATIS-1114. Change NettyServerStreamRpc to use only one ConcurrentMap.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #236:
URL: https://github.com/apache/incubator-ratis/pull/236#discussion_r513749758
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/NettyServerStreamRpc.java
##########
@@ -310,26 +324,21 @@ protected void encode(ChannelHandlerContext context, DataStreamReplyByteBuffer r
};
}
- private Channel getChannel() {
- return channelFuture.awaitUninterruptibly().channel();
- }
-
@Override
public void start() {
channelFuture.syncUninterruptibly();
}
@Override
public void close() {
- final ChannelFuture f = getChannel().close();
- f.syncUninterruptibly();
- bossGroup.shutdownGracefully(0, 100, TimeUnit.MILLISECONDS);
- workerGroup.shutdownGracefully(0, 100, TimeUnit.MILLISECONDS);
try {
+ channelFuture.channel().close().sync();
+ bossGroup.shutdownGracefully(0, 100, TimeUnit.MILLISECONDS);
+ workerGroup.shutdownGracefully(0, 100, TimeUnit.MILLISECONDS);
bossGroup.awaitTermination(1000, TimeUnit.MILLISECONDS);
workerGroup.awaitTermination(1000, TimeUnit.MILLISECONDS);
} catch (InterruptedException e) {
- LOG.error("Interrupt EventLoopGroup terminate", e);
+ LOG.error(this + ": Interrupted close()", e);
Review comment:
nit: `this + ".close() is Interrupted"`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] amaliujia commented on a change in pull request #236: RATIS-1114. Change NettyServerStreamRpc to use only one ConcurrentMap.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #236:
URL: https://github.com/apache/incubator-ratis/pull/236#discussion_r513749758
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/NettyServerStreamRpc.java
##########
@@ -310,26 +324,21 @@ protected void encode(ChannelHandlerContext context, DataStreamReplyByteBuffer r
};
}
- private Channel getChannel() {
- return channelFuture.awaitUninterruptibly().channel();
- }
-
@Override
public void start() {
channelFuture.syncUninterruptibly();
}
@Override
public void close() {
- final ChannelFuture f = getChannel().close();
- f.syncUninterruptibly();
- bossGroup.shutdownGracefully(0, 100, TimeUnit.MILLISECONDS);
- workerGroup.shutdownGracefully(0, 100, TimeUnit.MILLISECONDS);
try {
+ channelFuture.channel().close().sync();
+ bossGroup.shutdownGracefully(0, 100, TimeUnit.MILLISECONDS);
+ workerGroup.shutdownGracefully(0, 100, TimeUnit.MILLISECONDS);
bossGroup.awaitTermination(1000, TimeUnit.MILLISECONDS);
workerGroup.awaitTermination(1000, TimeUnit.MILLISECONDS);
} catch (InterruptedException e) {
- LOG.error("Interrupt EventLoopGroup terminate", e);
+ LOG.error(this + ": Interrupted close()", e);
Review comment:
nit: `this + ".close() is interrupted"`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] amaliujia commented on a change in pull request #236: RATIS-1114. Change NettyServerStreamRpc to use only one ConcurrentMap.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #236:
URL: https://github.com/apache/incubator-ratis/pull/236#discussion_r513742069
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/NettyServerStreamRpc.java
##########
@@ -170,12 +190,12 @@ public void addRaftPeers(Collection<RaftPeer> newPeers) {
proxies.addPeers(newPeers);
}
- private CompletableFuture<DataStream> getDataStreamFuture(ByteBuf buf) {
+ private StreamInfo newStreamInfo(ByteBuf buf) {
try {
- final RaftClientRequest request =
- ClientProtoUtils.toRaftClientRequest(RaftProtos.RaftClientRequestProto.parseFrom(buf.nioBuffer()));
- return stateMachine.data().stream(request);
- } catch (InvalidProtocolBufferException e) {
+ final RaftClientRequest request = ClientProtoUtils.toRaftClientRequest(
+ RaftClientRequestProto.parseFrom(buf.nioBuffer()));
+ return new StreamInfo(stateMachine.data().stream(request), proxies.getDataStreamOutput());
+ } catch (Throwable e) {
Review comment:
Out of curiosity: why not use `Exception e`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang commented on pull request #236: RATIS-1114. Change NettyServerStreamRpc to use only one ConcurrentMap.
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #236:
URL: https://github.com/apache/incubator-ratis/pull/236#issuecomment-718274928
@szetszwo Thanks the patch. I have merged it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] amaliujia commented on pull request #236: RATIS-1114. Change NettyServerStreamRpc to use only one ConcurrentMap.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #236:
URL: https://github.com/apache/incubator-ratis/pull/236#issuecomment-718201756
Overall looks good to me
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org