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/30 07:25:42 UTC
[GitHub] [incubator-ratis] szetszwo opened a new pull request #243: RATIS-1121. Support multiple streams.
szetszwo opened a new pull request #243:
URL: https://github.com/apache/incubator-ratis/pull/243
See https://issues.apache.org/jira/browse/RATIS-1121
----------------------------------------------------------------
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 #243: RATIS-1121. Support multiple streams.
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #243:
URL: https://github.com/apache/incubator-ratis/pull/243#discussion_r514930109
##########
File path: ratis-test/src/test/java/org/apache/ratis/datastream/TestDataStreamBase.java
##########
@@ -65,8 +84,7 @@ public int write(ByteBuffer src) {
}
final int remaining = src.remaining();
for(; src.remaining() > 0; ) {
- Assert.assertEquals(pos2byte(byteWritten), src.get());
- byteWritten += 1;
+ Assert.assertEquals(pos2byte(byteWritten.getAndIncrement()), src.get());
Review comment:
Sorry, why we use AtomicInteger for byteWritten? will `write(ByteBuffer src)` be called by multi-thread ? If so, can we use `synchronized write(ByteBuffer src)` ?
----------------------------------------------------------------
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 #243: RATIS-1121. Support multiple streams.
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #243:
URL: https://github.com/apache/incubator-ratis/pull/243#discussion_r514930109
##########
File path: ratis-test/src/test/java/org/apache/ratis/datastream/TestDataStreamBase.java
##########
@@ -65,8 +84,7 @@ public int write(ByteBuffer src) {
}
final int remaining = src.remaining();
for(; src.remaining() > 0; ) {
- Assert.assertEquals(pos2byte(byteWritten), src.get());
- byteWritten += 1;
+ Assert.assertEquals(pos2byte(byteWritten.getAndIncrement()), src.get());
Review comment:
Sorry, why we use AtomicInteger for byteWritten? will `write(ByteBuffer src)` be called by multi-thread ?
----------------------------------------------------------------
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 #243: RATIS-1121. Support multiple streams.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #243:
URL: https://github.com/apache/incubator-ratis/pull/243#discussion_r514961932
##########
File path: ratis-test/src/test/java/org/apache/ratis/datastream/TestDataStreamBase.java
##########
@@ -205,14 +228,19 @@ private void runTestDataStream(int bufferSize, int bufferNum) {
Assert.assertEquals(reply.getType(), Type.STREAM_DATA);
}
- for (SingleDataStreamStateMachine s : singleDataStreamStateMachines) {
- RaftClientRequest writeRequest = s.getWriteRequest();
- if (writeRequest.getClientId().equals(impl.getHeader().getClientId())) {
- Assert.assertEquals(writeRequest.getCallId(), impl.getHeader().getCallId());
- Assert.assertEquals(writeRequest.getRaftGroupId(), impl.getHeader().getRaftGroupId());
- Assert.assertEquals(writeRequest.getServerId(), impl.getHeader().getServerId());
+ final RaftClientRequest header = out.getHeader();
+ for (MultiDataStreamStateMachine s : stateMachines) {
+ final SingleDataStream stream = s.getSingleDataStream(header.getCallId());
+ if (stream == null) {
+ continue;
+ }
Review comment:
Assert.assertNotNull(stream) will fail the test when stream == null. It is a possible case when there are multiple servers.
----------------------------------------------------------------
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 #243: RATIS-1121. Support multiple streams.
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #243:
URL: https://github.com/apache/incubator-ratis/pull/243#discussion_r514930109
##########
File path: ratis-test/src/test/java/org/apache/ratis/datastream/TestDataStreamBase.java
##########
@@ -65,8 +84,7 @@ public int write(ByteBuffer src) {
}
final int remaining = src.remaining();
for(; src.remaining() > 0; ) {
- Assert.assertEquals(pos2byte(byteWritten), src.get());
- byteWritten += 1;
+ Assert.assertEquals(pos2byte(byteWritten.getAndIncrement()), src.get());
Review comment:
Sorry, why we use AtomicInteger for byteWritten, will `write(ByteBuffer src)` be called by multi-thread ?
----------------------------------------------------------------
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 #243: RATIS-1121. Support multiple streams.
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #243:
URL: https://github.com/apache/incubator-ratis/pull/243#discussion_r514930109
##########
File path: ratis-test/src/test/java/org/apache/ratis/datastream/TestDataStreamBase.java
##########
@@ -65,8 +84,7 @@ public int write(ByteBuffer src) {
}
final int remaining = src.remaining();
for(; src.remaining() > 0; ) {
- Assert.assertEquals(pos2byte(byteWritten), src.get());
- byteWritten += 1;
+ Assert.assertEquals(pos2byte(byteWritten.getAndIncrement()), src.get());
Review comment:
Sorry, can we use `synchronized write(ByteBuffer src)`, instead of use AtomicInteger for byteWritten?
----------------------------------------------------------------
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 #243: RATIS-1121. Support multiple streams.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #243:
URL: https://github.com/apache/incubator-ratis/pull/243#discussion_r514967283
##########
File path: ratis-test/src/test/java/org/apache/ratis/datastream/TestDataStreamBase.java
##########
@@ -65,8 +84,7 @@ public int write(ByteBuffer src) {
}
final int remaining = src.remaining();
for(; src.remaining() > 0; ) {
- Assert.assertEquals(pos2byte(byteWritten), src.get());
- byteWritten += 1;
+ Assert.assertEquals(pos2byte(byteWritten.getAndIncrement()), src.get());
Review comment:
Using int may work but it is hard to understand why it works. Since this is a test, we want to make sure that it won't has bugs. When the test fails, we don't want to ask questions like "do we need to synchronize byteWritten?" :)
----------------------------------------------------------------
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 #243: RATIS-1121. Support multiple streams.
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #243:
URL: https://github.com/apache/incubator-ratis/pull/243#discussion_r514930109
##########
File path: ratis-test/src/test/java/org/apache/ratis/datastream/TestDataStreamBase.java
##########
@@ -65,8 +84,7 @@ public int write(ByteBuffer src) {
}
final int remaining = src.remaining();
for(; src.remaining() > 0; ) {
- Assert.assertEquals(pos2byte(byteWritten), src.get());
- byteWritten += 1;
+ Assert.assertEquals(pos2byte(byteWritten.getAndIncrement()), src.get());
Review comment:
Sorry, do we need AtomicInteger for byteWritten?
----------------------------------------------------------------
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 #243: RATIS-1121. Support multiple streams.
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #243:
URL: https://github.com/apache/incubator-ratis/pull/243#discussion_r514969677
##########
File path: ratis-test/src/test/java/org/apache/ratis/datastream/TestDataStreamBase.java
##########
@@ -65,8 +84,7 @@ public int write(ByteBuffer src) {
}
final int remaining = src.remaining();
for(; src.remaining() > 0; ) {
- Assert.assertEquals(pos2byte(byteWritten), src.get());
- byteWritten += 1;
+ Assert.assertEquals(pos2byte(byteWritten.getAndIncrement()), src.get());
Review comment:
@szetszwo `public int write(ByteBuffer src) ` seems won't be called by multi-thread in NettyServerStreamRpc currently ?
----------------------------------------------------------------
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 #243: RATIS-1121. Support multiple streams.
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #243:
URL: https://github.com/apache/incubator-ratis/pull/243#discussion_r514920963
##########
File path: ratis-test/src/test/java/org/apache/ratis/datastream/TestDataStreamBase.java
##########
@@ -205,14 +228,19 @@ private void runTestDataStream(int bufferSize, int bufferNum) {
Assert.assertEquals(reply.getType(), Type.STREAM_DATA);
}
- for (SingleDataStreamStateMachine s : singleDataStreamStateMachines) {
- RaftClientRequest writeRequest = s.getWriteRequest();
- if (writeRequest.getClientId().equals(impl.getHeader().getClientId())) {
- Assert.assertEquals(writeRequest.getCallId(), impl.getHeader().getCallId());
- Assert.assertEquals(writeRequest.getRaftGroupId(), impl.getHeader().getRaftGroupId());
- Assert.assertEquals(writeRequest.getServerId(), impl.getHeader().getServerId());
+ final RaftClientRequest header = out.getHeader();
+ for (MultiDataStreamStateMachine s : stateMachines) {
+ final SingleDataStream stream = s.getSingleDataStream(header.getCallId());
+ if (stream == null) {
+ continue;
+ }
Review comment:
Maybe we can `Assert.assertNotNull(stream)` instead of `continue `?
----------------------------------------------------------------
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