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/11/30 13:52:13 UTC
[GitHub] [incubator-ratis] runzhiwang opened a new pull request #308: RATIS-1178. Use RaftClient to submit request
runzhiwang opened a new pull request #308:
URL: https://github.com/apache/incubator-ratis/pull/308
## 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.
----------------------------------------------------------------
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 #308: RATIS-1178. Use RaftClient to submit request
Posted by GitBox <gi...@apache.org>.
runzhiwang merged pull request #308:
URL: https://github.com/apache/incubator-ratis/pull/308
----------------------------------------------------------------
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 a change in pull request #308: RATIS-1178. Use RaftClient to submit request
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #308:
URL: https://github.com/apache/incubator-ratis/pull/308#discussion_r533102773
##########
File path: ratis-test/src/test/java/org/apache/ratis/datastream/DataStreamTestUtils.java
##########
@@ -343,10 +343,8 @@ static void assertHeader(RaftServer server, RaftClientRequest header, int dataSi
static void assertRaftClientMessage(RaftClientMessage expected, RaftPeerId expectedServerId, RaftClientMessage computed) {
Assert.assertNotNull(computed);
- Assert.assertEquals(expected.getClientId(), computed.getClientId());
Review comment:
> In more details, Primary receives a RaftClientRequest R from a client as usual. Then, it encodes R as a Message, puts it in a new RaftClientRequest F and sends it to the Leader. Since Leader sees that the Type is ForwarededRequestTypeProto, it will decode the Message back to R and then processes R.
RaftClientRequest R -> primary's client wrap RaftClientRequest R in RaftClientRequest F -> leader decode R from F -> leader response RaftClientReply of R -> leader convert RaftClientReply of R to RaftClientReply of F.
@szetszwo Because the ClientId and CallId of F's RaftClientReply belongs to primary's client. So we can not check the ClientId and CallId of F's RaftClientReply.
----------------------------------------------------------------
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 closed pull request #308: RATIS-1178. Use RaftClient to submit request
Posted by GitBox <gi...@apache.org>.
runzhiwang closed pull request #308:
URL: https://github.com/apache/incubator-ratis/pull/308
----------------------------------------------------------------
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 closed pull request #308: RATIS-1178. Use RaftClient to submit request
Posted by GitBox <gi...@apache.org>.
runzhiwang closed pull request #308:
URL: https://github.com/apache/incubator-ratis/pull/308
----------------------------------------------------------------
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 #308: RATIS-1178. Use RaftClient to submit request
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #308:
URL: https://github.com/apache/incubator-ratis/pull/308#issuecomment-736587463
@szetszwo Thanks for review. I have merged the patch, because CI passed.
----------------------------------------------------------------
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] szetszwo commented on a change in pull request #308: RATIS-1178. Use RaftClient to submit request
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #308:
URL: https://github.com/apache/incubator-ratis/pull/308#discussion_r533160847
##########
File path: ratis-test/src/test/java/org/apache/ratis/datastream/DataStreamTestUtils.java
##########
@@ -343,10 +343,8 @@ static void assertHeader(RaftServer server, RaftClientRequest header, int dataSi
static void assertRaftClientMessage(RaftClientMessage expected, RaftPeerId expectedServerId, RaftClientMessage computed) {
Assert.assertNotNull(computed);
- Assert.assertEquals(expected.getClientId(), computed.getClientId());
Review comment:
Similar to expectedServerId, let's pass the clientId in Primary as the expectedClientId (a new parameter).
##########
File path: ratis-client/src/main/java/org/apache/ratis/client/RaftClient.java
##########
@@ -53,8 +52,8 @@
/** Get the {@link GroupManagementApi} for the given server. */
GroupManagementApi getGroupManagementApi(RaftPeerId server);
- /** Get the {@link AsyncApi}. */
- AsyncApi async();
+ /** Get the {@link AsyncRpcApi}. */
+ AsyncRpcApi async();
Review comment:
We should keep returning AsyncApi in order to hide AsyncRpcApi. We will cast it to AsyncRpcApi in our code.
----------------------------------------------------------------
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] szetszwo commented on a change in pull request #308: RATIS-1178. Use RaftClient to submit request
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #308:
URL: https://github.com/apache/incubator-ratis/pull/308#discussion_r532617176
##########
File path: ratis-netty/src/main/java/org/apache/ratis/netty/server/DataStreamManagement.java
##########
@@ -61,11 +64,12 @@
import java.util.concurrent.atomic.AtomicReference;
import java.util.function.Function;
import java.util.stream.Collectors;
-import java.util.stream.Stream;
public class DataStreamManagement {
public static final Logger LOG = LoggerFactory.getLogger(DataStreamManagement.class);
+ private Map<RaftGroupId, RaftClient> clientMap = new ConcurrentHashMap<>();
Review comment:
clientMap is unused.
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -145,6 +152,16 @@
getMemberId(), () -> commitInfoCache::get, () -> retryCache);
this.startComplete = new AtomicBoolean(false);
+
+ this.raftClient = JavaUtils.memoize(() -> {
+ final RaftProperties p = new RaftProperties();
+ RaftConfigKeys.Rpc.setType(p, SupportedRpcType.GRPC);
Review 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());
```
##########
File path: ratis-proto/src/main/proto/Raft.proto
##########
@@ -73,6 +73,7 @@ message StateMachineLogEntryProto {
enum Type {
WRITE = 0;
DATASTREAM = 1;
+ FORWARD = 2;
Review comment:
It seems not needed since StateMachine only sees DATASTREAM requests but not FORWARD requests.
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -719,10 +749,16 @@ public void stepDownOnJvmPause() {
} else {
final RetryCache.CacheEntry cacheEntry = previousResult.getEntry();
+ RaftClientRequest dataStreamRequest = null;
+ if (request.is(TypeCase.FORWARD)) {
+ dataStreamRequest = getDataStreamRaftClientRequest(request);
Review comment:
Just use
```
request = getDataStreamRaftClientRequest(request);
```
##########
File path: ratis-client/src/main/java/org/apache/ratis/client/api/AsyncApi.java
##########
@@ -40,6 +40,8 @@
*/
CompletableFuture<RaftClientReply> send(Message message);
+ CompletableFuture<RaftClientReply> sendForward(Message message);
Review comment:
Similar to DataStreamRpcApi, we should add an AsyncRpcApi so that the new method could be hidden from users.
The parameter should be a RaftClientRequest.
```
public CompletableFuture<RaftClientReply> sendForward(RaftClientRequest request);
```
----------------------------------------------------------------
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 a change in pull request #308: RATIS-1178. Use RaftClient to submit request
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #308:
URL: https://github.com/apache/incubator-ratis/pull/308#discussion_r533333866
##########
File path: ratis-test/src/test/java/org/apache/ratis/datastream/DataStreamTestUtils.java
##########
@@ -343,10 +343,8 @@ static void assertHeader(RaftServer server, RaftClientRequest header, int dataSi
static void assertRaftClientMessage(RaftClientMessage expected, RaftPeerId expectedServerId, RaftClientMessage computed) {
Assert.assertNotNull(computed);
- Assert.assertEquals(expected.getClientId(), computed.getClientId());
Review comment:
@szetszwo Thanks the suggestions. I have updated the patch.
As you find, if we expose primary's ClientId to user, the test looks complicated. Besides, maybe we should not expose primary's ClientId to user, it maybe confuse user, because user will find RaftClientReply's ClientId does not match his RaftClient's ClientId. So maybe primary can convert RaftClientReply of F to RaftClientReply of R again. what do you think ?
----------------------------------------------------------------
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] szetszwo commented on a change in pull request #308: RATIS-1178. Use RaftClient to submit request
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #308:
URL: https://github.com/apache/incubator-ratis/pull/308#discussion_r533357281
##########
File path: ratis-test/src/test/java/org/apache/ratis/datastream/DataStreamTestUtils.java
##########
@@ -343,10 +343,8 @@ static void assertHeader(RaftServer server, RaftClientRequest header, int dataSi
static void assertRaftClientMessage(RaftClientMessage expected, RaftPeerId expectedServerId, RaftClientMessage computed) {
Assert.assertNotNull(computed);
- Assert.assertEquals(expected.getClientId(), computed.getClientId());
Review comment:
In the test, we may pass the primary server in the builder.
```
+++ b/ratis-test/src/test/java/org/apache/ratis/datastream/DataStreamAsyncClusterTests.java
@@ -106,8 +106,7 @@ public abstract class DataStreamAsyncClusterTests<CLUSTER extends MiniRaftCluste
.orElseThrow(IllegalStateException::new);
}
- ClientId getPrimaryClientId(CLUSTER cluster, RaftClient client) {
- RaftPeer primary = client.getPrimaryDataStreamServer();
+ ClientId getPrimaryClientId(CLUSTER cluster, RaftPeer primary) {
return cluster.getDivision(primary.getId()).getRaftClient().getId();
}
@@ -115,8 +114,9 @@ public abstract class DataStreamAsyncClusterTests<CLUSTER extends MiniRaftCluste
final Iterable<RaftServer> servers = CollectionUtils.as(cluster.getServers(), s -> s);
final RaftPeerId leader = cluster.getLeader().getId();
final List<CompletableFuture<RaftClientReply>> futures = new ArrayList<>();
- try(RaftClient client = cluster.createClient()) {
- ClientId primaryClientId = getPrimaryClientId(cluster, client);
+ final RaftPeer primaryServer = CollectionUtils.random(cluster.getGroup().getPeers());
+ try(RaftClient client = cluster.createClient(primaryServer)) {
+ ClientId primaryClientId = getPrimaryClientId(cluster, primaryServer);
for (int i = 0; i < numStreams; i++) {
final DataStreamOutputImpl out = (DataStreamOutputImpl) client.getDataStreamApi().stream();
futures.add(CompletableFuture.supplyAsync(() -> DataStreamTestUtils.writeAndCloseAndAssertReplies(
```
----------------------------------------------------------------
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 a change in pull request #308: RATIS-1178. Use RaftClient to submit request
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #308:
URL: https://github.com/apache/incubator-ratis/pull/308#discussion_r533100887
##########
File path: ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java
##########
@@ -145,6 +152,16 @@
getMemberId(), () -> commitInfoCache::get, () -> retryCache);
this.startComplete = new AtomicBoolean(false);
+
+ this.raftClient = JavaUtils.memoize(() -> {
+ final RaftProperties p = new RaftProperties();
+ RaftConfigKeys.Rpc.setType(p, SupportedRpcType.GRPC);
Review 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)
```
----------------------------------------------------------------
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 a change in pull request #308: RATIS-1178. Use RaftClient to submit request
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #308:
URL: https://github.com/apache/incubator-ratis/pull/308#discussion_r533102773
##########
File path: ratis-test/src/test/java/org/apache/ratis/datastream/DataStreamTestUtils.java
##########
@@ -343,10 +343,8 @@ static void assertHeader(RaftServer server, RaftClientRequest header, int dataSi
static void assertRaftClientMessage(RaftClientMessage expected, RaftPeerId expectedServerId, RaftClientMessage computed) {
Assert.assertNotNull(computed);
- Assert.assertEquals(expected.getClientId(), computed.getClientId());
Review comment:
> In more details, Primary receives a RaftClientRequest R from a client as usual. Then, it encodes R as a Message, puts it in a new RaftClientRequest F and sends it to the Leader. Since Leader sees that the Type is ForwarededRequestTypeProto, it will decode the Message back to R and then processes R.
RaftClientRequest R -> primary's client wrap RaftClientRequest R in RaftClientRequest F -> leader decode R from F -> leader response RaftClientReply of R -> leader convert RaftClientReply of R to RaftClientReply of F.
@szetszwo Because the ClientId and CallId of F's RaftClientReply belongs to primary's client. So we can not check the ClientId and CallId of F's RaftClientReply. Or maybe primary can convert RaftClientReply of F to RaftClientReply of R again. what do you think ?
----------------------------------------------------------------
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 a change in pull request #308: RATIS-1178. Use RaftClient to submit request
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #308:
URL: https://github.com/apache/incubator-ratis/pull/308#discussion_r533102773
##########
File path: ratis-test/src/test/java/org/apache/ratis/datastream/DataStreamTestUtils.java
##########
@@ -343,10 +343,8 @@ static void assertHeader(RaftServer server, RaftClientRequest header, int dataSi
static void assertRaftClientMessage(RaftClientMessage expected, RaftPeerId expectedServerId, RaftClientMessage computed) {
Assert.assertNotNull(computed);
- Assert.assertEquals(expected.getClientId(), computed.getClientId());
Review comment:
> In more details, Primary receives a RaftClientRequest R from a client as usual. Then, it encodes R as a Message, puts it in a new RaftClientRequest F and sends it to the Leader. Since Leader sees that the Type is ForwarededRequestTypeProto, it will decode the Message back to R and then processes R.
RaftClientRequest R -> primary's client wrap RaftClientRequest R in RaftClientRequest F -> leader decode R from F -> leader response RaftClientReply of R -> leader convert RaftClientReply of R to RaftClientReply of F.
Because the ClientId and CallId of F's RaftClientReply belongs to primary's client. So we can not check the ClientId and CallId of F's RaftClientReply.
----------------------------------------------------------------
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 #308: RATIS-1178. Use RaftClient to submit request
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #308:
URL: https://github.com/apache/incubator-ratis/pull/308#issuecomment-735798814
@szetszwo Could you have a brief review ? If you agree with the idea, I will fix the failed ut.
----------------------------------------------------------------
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