RATIS-1178. Use RaftClient to submit request#308
RATIS-1178. Use RaftClient to submit request#308runzhiwang merged 4 commits intoapache:masterfrom runzhiwang:RaftClient-forward
Conversation
|
@szetszwo Could you have a brief review ? If you agree with the idea, I will fix the failed ut. |
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
The change looks great. It is much cleaner. Some comments inlined.
ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java
Outdated
Show resolved
Hide resolved
| final RaftProperties p = new RaftProperties(); | ||
| RaftConfigKeys.Rpc.setType(p, SupportedRpcType.GRPC); |
There was a problem hiding this comment.
It should call getRaftServer().getProperties(). Then, it will use all the existing settings. We don't have to set RPC type.
this.raftClient = JavaUtils.memoize(() -> RaftClient.newBuilder()
.setRaftGroup(group)
.setProperties(getRaftServer().getProperties())
.build());
There was a problem hiding this comment.
Currently, netty client did not implement async api, so I ignore TestNettyDataStreamWithNettyCluster, otherwise will throw UnsupportedOperationException.
Caused by: java.lang.UnsupportedOperationException: class org.apache.ratis.netty.client.NettyClientRpc does not support this method.
at org.apache.ratis.client.RaftClientRpc.sendRequestAsync(RaftClientRpc.java:35)
at org.apache.ratis.client.impl.OrderedAsync.sendRequest(OrderedAsync.java:234)
at org.apache.ratis.client.impl.OrderedAsync.sendRequestWithRetry(OrderedAsync.java:188)
at org.apache.ratis.util.SlidingWindow$Client.sendOrDelayRequest(SlidingWindow.java:278)
at org.apache.ratis.util.SlidingWindow$Client.submitNewRequest(SlidingWindow.java:257)
at org.apache.ratis.client.impl.OrderedAsync.send(OrderedAsync.java:169)
at org.apache.ratis.client.impl.OrderedAsync.newInstance(OrderedAsync.java:117)
at org.apache.ratis.client.impl.RaftClientImpl.lambda$new$0(RaftClientImpl.java:154)
at org.apache.ratis.util.MemoizedSupplier.get(MemoizedSupplier.java:62)
at org.apache.ratis.client.impl.RaftClientImpl.getOrderedAsync(RaftClientImpl.java:206)
at org.apache.ratis.client.impl.AsyncImpl.send(AsyncImpl.java:41)
at org.apache.ratis.client.impl.AsyncImpl.sendForward(AsyncImpl.java:67)
at org.apache.ratis.netty.server.DataStreamManagement.startTransaction(DataStreamManagement.java:304)
at org.apache.ratis.netty.server.DataStreamManagement.lambda$null$7(DataStreamManagement.java:384)
at java.util.concurrent.CompletableFuture.biApply(CompletableFuture.java:1105)
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
Outdated
Show resolved
Hide resolved
ratis-client/src/main/java/org/apache/ratis/client/api/AsyncApi.java
Outdated
Show resolved
Hide resolved
ratis-test/src/test/java/org/apache/ratis/datastream/DataStreamTestUtils.java
Show resolved
Hide resolved
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
The change looks good. Just some minor comments.
ratis-test/src/test/java/org/apache/ratis/datastream/DataStreamTestUtils.java
Show resolved
Hide resolved
ratis-client/src/main/java/org/apache/ratis/client/RaftClient.java
Outdated
Show resolved
Hide resolved
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
+1 the change looks good.
|
@szetszwo Thanks for review. I have merged the patch, because CI passed. |
|
|
||
| DataStreamMap getDataStreamMap(); | ||
|
|
||
| RaftClient getRaftClient(); |
There was a problem hiding this comment.
@szetszwo it seems that we no longer use this method to submit request. If it's the case, I'm willing to figure out whether we can drop this "dead code".
There was a problem hiding this comment.
You are right that we may remove it. But we need to keep it in branch-2 for compatibility.
There was a problem hiding this comment.
OK. I submitted #886 already that is against master (3.0). I'll file a JIRA ticket later.
* RATIS-1178. Use RaftClient to submit request * fix code review * fix code review * fix code review
3 participants
What changes were proposed in this pull request?
Use RaftClient to submit request
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-1178
How was this patch tested?
No need to add new ut.