You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by GitBox <gi...@apache.org> on 2022/06/30 02:30:09 UTC
[GitHub] [ratis] ChenSammi opened a new pull request, #663: RATIS-1605. Refactor MiniRaftCluster#addNewPeers.
ChenSammi opened a new pull request, #663:
URL: https://github.com/apache/ratis/pull/663
https://issues.apache.org/jira/browse/RATIS-1605
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] ChenSammi commented on pull request #663: RATIS-1605. Do not start RaftServer in MiniRaftCluster#addNewPeers when startNewPeer is false
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #663:
URL: https://github.com/apache/ratis/pull/663#issuecomment-1185100406
Thanks @szetszwo .
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] ChenSammi commented on pull request #663: RATIS-1605. Refactor MiniRaftCluster#addNewPeers.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #663:
URL: https://github.com/apache/ratis/pull/663#issuecomment-1184453866
The failed TestNettyDataStreamChainTopologyWithGrpcCluster#testMultipleStreamsMultipleServersStepDownLeader seems irrelevant. It can pass locally.
@szetszwo would you like to take another look of this patch?
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] ChenSammi commented on pull request #663: RATIS-1605. Refactor MiniRaftCluster#addNewPeers.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #663:
URL: https://github.com/apache/ratis/pull/663#issuecomment-1185099762
> Do not start RaftServer in MiniRaftCluster#addNewPeers
This looks good.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] ChenSammi commented on pull request #663: RATIS-1605. Refactor MiniRaftCluster#addNewPeers.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #663:
URL: https://github.com/apache/ratis/pull/663#issuecomment-1170808908
@szetszwo could you help to merge the PR? I don't have the privilege.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] ChenSammi commented on pull request #663: RATIS-1605. Refactor MiniRaftCluster#addNewPeers.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #663:
URL: https://github.com/apache/ratis/pull/663#issuecomment-1170836384
> @ChenSammi , just have rerun the failed jobs.
The failed test is because of the LOCK issue in RaftStorageDirectoryImpl. RaftStorageDirectoryImpl holds the LOCK, but it doesn't get chance to release it when RaftServer lifecycle from NEW->CLOSE. I will try to refactor it in next PR.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #663: RATIS-1605. Refactor MiniRaftCluster#addNewPeers.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #663:
URL: https://github.com/apache/ratis/pull/663#issuecomment-1185095191
How about "Do not start RaftServer in MiniRaftCluster#addNewPeers"?
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] ChenSammi commented on pull request #663: RATIS-1605. Refactor MiniRaftCluster#addNewPeers.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #663:
URL: https://github.com/apache/ratis/pull/663#issuecomment-1185076690
> @ChenSammi , the change looks good. Just a question inlined.
>
> > RATIS-1605. Refactor MiniRaftCluster#addNewPeers.
>
> BTW, the title above is no longer accurate. Could you update it?
Is there a proposed one? I am open to update 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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] ChenSammi commented on pull request #663: RATIS-1605. Refactor MiniRaftCluster#addNewPeers.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #663:
URL: https://github.com/apache/ratis/pull/663#issuecomment-1185094418
> @ChenSammi , how about changing the commit message to "Improve error and log messages"?
Will it be too generic? The core logic change in this patch is remove the RaftServer start process when addNewPeers is called with start as false. With this behavior change, some unit cases failed. The error improvement and log messages are mostly for this.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #663: RATIS-1605. Refactor MiniRaftCluster#addNewPeers.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #663:
URL: https://github.com/apache/ratis/pull/663#issuecomment-1170886782
> The failed test is because of the LOCK issue in RaftStorageDirectoryImpl. RaftStorageDirectoryImpl holds the LOCK, but it doesn't get chance to release it when RaftServer lifecycle from NEW->CLOSE. I will try to refactor it in next PR.
The failure seems related to this change since the tests did not fail in the other pull requests. We should fix the tests here. Otherwise, if we merge the current change, the tests will fail for other pull requests.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo merged pull request #663: RATIS-1605. Do not start RaftServer in MiniRaftCluster#addNewPeers when startNewPeer is false
Posted by GitBox <gi...@apache.org>.
szetszwo merged PR #663:
URL: https://github.com/apache/ratis/pull/663
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #663: RATIS-1605. Refactor MiniRaftCluster#addNewPeers.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #663:
URL: https://github.com/apache/ratis/pull/663#issuecomment-1185085896
@ChenSammi , how about changing the commit message to "Improve error and log messages"?
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #663: RATIS-1605. Refactor MiniRaftCluster#addNewPeers.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #663:
URL: https://github.com/apache/ratis/pull/663#issuecomment-1170834792
@ChenSammi , just have rerun the failed jobs.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] ChenSammi commented on pull request #663: RATIS-1605. Refactor MiniRaftCluster#addNewPeers.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #663:
URL: https://github.com/apache/ratis/pull/663#issuecomment-1184361516
Failed UT
testKillLeaderDuringReconf(org.apache.ratis.netty.TestRaftReconfigurationWithNetty) Time elapsed: 0.6 s <<< ERROR!
java.lang.IllegalStateException: s3: Failed to getChannel since the service is not yet started
at org.apache.ratis.netty.server.NettyRpcService.getChannel(NettyRpcService.java:138)
at org.apache.ratis.netty.server.NettyRpcService.getInetSocketAddress(NettyRpcService.java:170)
at org.apache.ratis.server.impl.RaftServerProxy.buildRaftPeer(RaftServerProxy.java:305)
at org.apache.ratis.util.MemoizedSupplier.get(MemoizedSupplier.java:62)
at org.apache.ratis.server.impl.RaftServerProxy.getPeer(RaftServerProxy.java:299)
at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193)
at java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1384)
at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:482)
at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:472)
at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:708)
at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
at java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:566)
at org.apache.ratis.server.impl.MiniRaftCluster.toRaftPeers(MiniRaftCluster.java:403)
at org.apache.ratis.server.impl.MiniRaftCluster.addNewPeers(MiniRaftCluster.java:442)
at org.apache.ratis.server.impl.MiniRaftCluster.addNewPeers(MiniRaftCluster.java:408)
at org.apache.ratis.server.impl.RaftReconfigurationBaseTest.runTestKillLeaderDuringReconf(RaftReconfigurationBaseTest.java:422)
at org.apache.ratis.server.impl.MiniRaftCluster$Factory$Get.runWithNewCluster(MiniRaftCluster.java:125)
at org.apache.ratis.server.impl.MiniRaftCluster$Factory$Get.runWithNewCluster(MiniRaftCluster.java:113)
at org.apache.ratis.server.impl.RaftReconfigurationBaseTest.testKillLeaderDuringReconf(RaftReconfigurationBaseTest.java:413)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:299)
at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:293)
at java.util.concurrent.FutureTask.run(FutureTask.java:266)
at java.lang.Thread.run(Thread.java:750)
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] ChenSammi commented on a diff in pull request #663: RATIS-1605. Refactor MiniRaftCluster#addNewPeers.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on code in PR #663:
URL: https://github.com/apache/ratis/pull/663#discussion_r921728948
##########
ratis-netty/src/main/java/org/apache/ratis/netty/server/NettyRpcService.java:
##########
@@ -167,7 +168,14 @@ public void closeImpl() throws IOException {
@Override
public InetSocketAddress getInetSocketAddress() {
- return (InetSocketAddress)getChannel().localAddress();
+ try {
+ return (InetSocketAddress) getChannel().localAddress();
+ } catch (IllegalStateException e) {
+ if (socketAddress.getPort() != NettyConfigKeys.Server.PORT_DEFAULT) {
+ return socketAddress;
+ }
Review Comment:
Because if the port is PORT_DEFAULT which is 0, it's not a real port, cannot be returned to construct the RaftPeer object.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a diff in pull request #663: RATIS-1605. Refactor MiniRaftCluster#addNewPeers.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #663:
URL: https://github.com/apache/ratis/pull/663#discussion_r921356556
##########
ratis-netty/src/main/java/org/apache/ratis/netty/server/NettyRpcService.java:
##########
@@ -167,7 +168,14 @@ public void closeImpl() throws IOException {
@Override
public InetSocketAddress getInetSocketAddress() {
- return (InetSocketAddress)getChannel().localAddress();
+ try {
+ return (InetSocketAddress) getChannel().localAddress();
+ } catch (IllegalStateException e) {
+ if (socketAddress.getPort() != NettyConfigKeys.Server.PORT_DEFAULT) {
+ return socketAddress;
+ }
Review Comment:
When `socketAddress.getPort() == NettyConfigKeys.Server.PORT_DEFAULT)`, why not returning `socketAddress`?
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a diff in pull request #663: RATIS-1605. Refactor MiniRaftCluster#addNewPeers.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #663:
URL: https://github.com/apache/ratis/pull/663#discussion_r921737211
##########
ratis-netty/src/main/java/org/apache/ratis/netty/server/NettyRpcService.java:
##########
@@ -167,7 +168,14 @@ public void closeImpl() throws IOException {
@Override
public InetSocketAddress getInetSocketAddress() {
- return (InetSocketAddress)getChannel().localAddress();
+ try {
+ return (InetSocketAddress) getChannel().localAddress();
+ } catch (IllegalStateException e) {
+ if (socketAddress.getPort() != NettyConfigKeys.Server.PORT_DEFAULT) {
+ return socketAddress;
+ }
Review Comment:
I see. Thanks.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org