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/06 02:58:25 UTC
[GitHub] [incubator-ratis] runzhiwang opened a new pull request #258: RATIS-1133. Primary and peer should use the same RaftClientReques
runzhiwang opened a new pull request #258:
URL: https://github.com/apache/incubator-ratis/pull/258
## What changes were proposed in this pull request?
When primary create DataStreamOutputImpl for peer, it will create a new [RaftClientRequest](https://github.com/apache/incubator-ratis/blob/master/ratis-client/src/main/java/org/apache/ratis/client/impl/DataStreamClientImpl.java#L77) and send it to peer. I think primary should send RaftClientRequest got from client to peer.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-1133
## How was this patch tested?
TODO
----------------------------------------------------------------
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 edited a comment on pull request #258: RATIS-1133. Primary and peer should use the same RaftClientReques
Posted by GitBox <gi...@apache.org>.
runzhiwang edited a comment on pull request #258:
URL: https://github.com/apache/incubator-ratis/pull/258#issuecomment-722779091
@szetszwo Could you help review this ? If you agree the change, I will add unit test.
----------------------------------------------------------------
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 merged pull request #258: RATIS-1133. Primary and peer should use the same RaftClientReques
Posted by GitBox <gi...@apache.org>.
szetszwo merged pull request #258:
URL: https://github.com/apache/incubator-ratis/pull/258
----------------------------------------------------------------
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 #258: RATIS-1133. Primary and peer should use the same RaftClientReques
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #258:
URL: https://github.com/apache/incubator-ratis/pull/258#issuecomment-722851091
@szetszwo Thanks for review. I have updated the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang commented on pull request #258: RATIS-1133. Primary and peer should use the same RaftClientReques
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #258:
URL: https://github.com/apache/incubator-ratis/pull/258#issuecomment-722779091
@szetszwo Could you help review this ? If agree with the change, I will add unit test.
----------------------------------------------------------------
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 edited a comment on pull request #258: RATIS-1133. Primary and peer should use the same RaftClientReques
Posted by GitBox <gi...@apache.org>.
runzhiwang edited a comment on pull request #258:
URL: https://github.com/apache/incubator-ratis/pull/258#issuecomment-722779091
@szetszwo Could you help review 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-ratis] runzhiwang edited a comment on pull request #258: RATIS-1133. Primary and peer should use the same RaftClientReques
Posted by GitBox <gi...@apache.org>.
runzhiwang edited a comment on pull request #258:
URL: https://github.com/apache/incubator-ratis/pull/258#issuecomment-722779091
----------------------------------------------------------------
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 #258: RATIS-1133. Primary and peer should use the same RaftClientReques
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #258:
URL: https://github.com/apache/incubator-ratis/pull/258#discussion_r518506478
##########
File path: ratis-test/src/test/java/org/apache/ratis/datastream/DataStreamBaseTest.java
##########
@@ -474,6 +474,7 @@ void assertHeader(Server server, RaftClientRequest header, int dataSize) throws
Assert.assertEquals(raftGroup.getGroupId(), header.getRaftGroupId());
Assert.assertEquals(dataSize, stream.getByteWritten());
Assert.assertEquals(writeRequest.getCallId(), header.getCallId());
+ Assert.assertEquals(writeRequest.getClientId(), header.getClientId());
final Server primary = getPrimaryServer();
if (server == primary) {
Review comment:
We may remove the if-statement and check all servers.
```
Assert.assertEquals(writeRequest.getServerId(), header.getServerId());
```
##########
File path: ratis-client/src/main/java/org/apache/ratis/client/impl/DataStreamClientImpl.java
##########
@@ -70,13 +70,13 @@ public DataStreamClientImpl(
private long streamOffset = 0;
public DataStreamOutputImpl(RaftGroupId groupId) {
- this(groupId, RaftClientImpl.nextCallId());
+ this(new RaftClientRequest(clientId, raftServer.getId(), groupId, RaftClientImpl.nextCallId(),
+ RaftClientRequest.writeRequestType()));
}
- public DataStreamOutputImpl(RaftGroupId groupId, long streamId) {
- this.header = new RaftClientRequest(clientId, raftServer.getId(), groupId, streamId,
- RaftClientRequest.writeRequestType());
- this.headerFuture = orderedStreamAsync.sendRequest(streamId, -1,
+ public DataStreamOutputImpl(RaftClientRequest request) {
Review comment:
We should use private.
##########
File path: ratis-client/src/main/java/org/apache/ratis/client/impl/DataStreamClientImpl.java
##########
@@ -70,13 +70,13 @@ public DataStreamClientImpl(
private long streamOffset = 0;
public DataStreamOutputImpl(RaftGroupId groupId) {
- this(groupId, RaftClientImpl.nextCallId());
+ this(new RaftClientRequest(clientId, raftServer.getId(), groupId, RaftClientImpl.nextCallId(),
+ RaftClientRequest.writeRequestType()));
}
Review comment:
Let's remove this constructor and move the new RaftClientRequest(..) to stream(RaftGroupId gid).
```
@Override
public DataStreamOutputRpc stream(RaftGroupId gid) {
return stream(new RaftClientRequest(clientId, raftServer.getId(), groupId, RaftClientImpl.nextCallId(),
RaftClientRequest.writeRequestType()));
}
```
----------------------------------------------------------------
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