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/24 10:11:08 UTC
[GitHub] [incubator-ratis] szetszwo opened a new pull request #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
szetszwo opened a new pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296
See https://issues.apache.org/jira/browse/RATIS-1175
----------------------------------------------------------------
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 #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
Posted by GitBox <gi...@apache.org>.
runzhiwang edited a comment on pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296#issuecomment-732968122
transferTo can avoid reading data to user space, it transfer data between kernel space and socket/disk.
1. But we still need to calculate checksum, so client still need read data to user space.
2. Besides, if we use transferTo, client transfer data to three nodes in parallel maybe best, otherwise if we use client -> primary -> peer, primary must wait finish write 128MB block, and then transfer the whole block file to follower, it maybe slow, but if client transfer data to three nodes in parallel, "the client has a higher network bandwidth requirement and it cannot optimise the network traffic".
So not sure whether we can use transferTo in ozone.
----------------------------------------------------------------
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 pull request #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296#issuecomment-732937162
@runzhiwang , good observation. I guess it depends on the ByteBuffer type in the source. If it is direct buffer, it will use transferToDirectly. The test uses FileInputStream which probably is not using direct buffer.
----------------------------------------------------------------
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 pull request #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296#issuecomment-732992036
@runzhiwang , thanks a lot for reviewing this. Let's continue the discussion on https://issues.apache.org/jira/browse/RATIS-1176
----------------------------------------------------------------
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 #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296#issuecomment-732925118
@szetszwo Hi, in transferTo method there are three methods: transferToDirectly which use sendfile and is the fastest, transferToTrustedChannel which use MapByteBuffer, transferToArbitraryChannel which use HeapByteBuffer and is the slowest. Our code use the slowest transferToArbitraryChannel , which will use HeapByteBuffer as the image shows. Not sure whether can we support transferToDirectly or transferToTrustedChannel.
----------------------------------------------------------------
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 pull request #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296#issuecomment-732990668
> But we need to calculate checksum, so client still need read data, to avoid reading data to user space, we can use MapByteBuffer. so we need 2 times read data from disk to kernel space: transferTo and MapByteBuffer.
We may pass MapByteBuffer to our writeAsync(..) method. We should benchmark it. I suspect it is better than transferTo(..) since it is using transferToArbitraryChannel(..).
----------------------------------------------------------------
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 #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296#issuecomment-732968122
transferTo can avoid reading data to user space, it transfer data between kernel space and socket/disk.
But we still need to calculate checksum, so client still need read data to user space. And if we use transferTo, client transfer data to three nodes in parallel maybe best, otherwise if we use client -> primary -> peer, primary must wait finish write 128MB block, and then transfer the block file to follower, it maybe slow, but if client transfer data to three nodes in parallel, "the client has a higher network bandwidth requirement and it cannot optimise the network traffic". So not sure whether can use transferTo in ozone.
----------------------------------------------------------------
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 pull request #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296#issuecomment-732944191
@runzhiwang , just found that our WritableByteChannel should at least extend SelectableChannel. Then, it may be able to use transferToDirectly. This needs some research.
How about we commit this first and continue the work in another JIRA?
----------------------------------------------------------------
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 #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
Posted by GitBox <gi...@apache.org>.
runzhiwang edited a comment on pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296#issuecomment-732968122
transferTo can avoid reading data to user space, it transfer data between kernel space and socket/disk.
1. But we need to calculate checksum, so client still need read data to user space.
2. Besides, if we use transferTo, client transfer data to three nodes in parallel maybe best, otherwise if we use client -> primary -> peer, primary must wait finish write 128MB block, and then transfer the whole block file to follower, it maybe slow, but if client transfer data to three nodes in parallel, "the client has a higher network bandwidth requirement and it cannot optimise the network traffic".
So not sure whether we can use transferTo in ozone.
----------------------------------------------------------------
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 closed pull request #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
Posted by GitBox <gi...@apache.org>.
szetszwo closed pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296
----------------------------------------------------------------
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 #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
Posted by GitBox <gi...@apache.org>.
runzhiwang edited a comment on pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296#issuecomment-732963170
Sorry my bad, transferTo support offset, for big file, we can also use transferTo to split it into multi blocks.
----------------------------------------------------------------
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 #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296#issuecomment-732960300
@szetszwo I agree. Do you think it can be used in ozone case, maybe can be used to write file smaller than 128MB, i.e. the size of block ? For file bigger than 128MB, we must split it into multi blocks, and can not use transferTo.
----------------------------------------------------------------
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 pull request #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296#issuecomment-732955407
Another way is to provide a different API for transferring files, say
```
CompletableFuture<DataStreamReply> writeAsync(RandomAccessFile);
```
Then, we may use Netty DefaultFileRegion for zero-copy file transfer.
https://github.com/netty/netty/blob/4.1/example/src/main/java/io/netty/example/file/FileServerHandler.java#L53
----------------------------------------------------------------
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 #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
Posted by GitBox <gi...@apache.org>.
runzhiwang edited a comment on pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296#issuecomment-732968122
transferTo can avoid reading data to user space, it transfer data between kernel space and socket/disk.
1. But we still need to calculate checksum, so client still need read data to user space.
2. Besides, if we use transferTo, client transfer data to three nodes in parallel maybe best, otherwise if we use client -> primary -> peer, primary must wait finish write 128MB block, and then transfer the block file to follower, it maybe slow, but if client transfer data to three nodes in parallel, "the client has a higher network bandwidth requirement and it cannot optimise the network traffic". So not sure whether can use transferTo in ozone.
----------------------------------------------------------------
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 #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
Posted by GitBox <gi...@apache.org>.
runzhiwang edited a comment on pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296#issuecomment-732963170
Sorry my bad, transferTo support offset (i.e. the parameter position), for big file, we can also use transferTo to split it into multi blocks.
```
public abstract long transferTo(long position, long count,
WritableByteChannel target)
```
----------------------------------------------------------------
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 #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296#issuecomment-732944678
@szetszwo Sure, thanks for explanation.
----------------------------------------------------------------
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 #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296#issuecomment-732963170
Sorry my bad, transferTo support offset, for big file, we can also use transferTo to write multi blocks.
----------------------------------------------------------------
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 #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
Posted by GitBox <gi...@apache.org>.
runzhiwang edited a comment on pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296#issuecomment-732925118
@szetszwo Hi, in transferTo method there are three methods: transferToDirectly which use sendfile and is the fastest, transferToTrustedChannel which use MapByteBuffer, transferToArbitraryChannel which use HeapByteBuffer and is the slowest. Our code use the slowest transferToArbitraryChannel , which will use HeapByteBuffer as the image shows. Not sure whether can we support transferToDirectly or transferToTrustedChannel, maybe we can not use netty if we want to support transferToDirectly.
----------------------------------------------------------------
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 #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on a change in pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296#discussion_r529498029
##########
File path: ratis-client/src/main/java/org/apache/ratis/client/impl/DataStreamClientImpl.java
##########
@@ -22,25 +22,39 @@
import org.apache.ratis.client.DataStreamOutputRpc;
import org.apache.ratis.conf.RaftProperties;
import org.apache.ratis.datastream.impl.DataStreamPacketByteBuffer;
+import org.apache.ratis.datastream.impl.DataStreamReplyByteBuffer;
import org.apache.ratis.proto.RaftProtos.DataStreamPacketHeaderProto.Type;
import org.apache.ratis.protocol.ClientId;
+import org.apache.ratis.protocol.ClientInvocationId;
import org.apache.ratis.protocol.DataStreamReply;
+import org.apache.ratis.protocol.RaftClientReply;
import org.apache.ratis.protocol.RaftClientRequest;
import org.apache.ratis.protocol.RaftGroupId;
import org.apache.ratis.protocol.RaftPeer;
import org.apache.ratis.protocol.exceptions.AlreadyClosedException;
+import org.apache.ratis.util.IOUtils;
import org.apache.ratis.util.JavaUtils;
import org.apache.ratis.util.MemoizedSupplier;
import java.io.IOException;
import java.nio.ByteBuffer;
+import java.nio.channels.WritableByteChannel;
import java.util.concurrent.CompletableFuture;
/**
* Streaming client implementation
* allows client to create streams and send asynchronously.
*/
public class DataStreamClientImpl implements DataStreamClient {
+ private static RaftClientReply getRaftClientReply(DataStreamReply reply) {
Review comment:
Thanks. Will move them to ClientProtoUtils.
----------------------------------------------------------------
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 #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
Posted by GitBox <gi...@apache.org>.
runzhiwang closed pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296
----------------------------------------------------------------
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 pull request #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296#issuecomment-732988191
> ... , otherwise if we use client -> primary -> peer, primary must wait finish write 128MB block, and then transfer the whole block file to follower ...
No. The transferTo method calls WritableByteChannel.write(..) multiple times. For example, transferToArbitraryChannel(..) create a 8KB buffer and call WritableByteChannel.write(..) every 8KB. We must send small packets in Streaming.
```
//sun.nio.ch.FileChannelImpl
private long transferToArbitraryChannel(long var1, int var3, WritableByteChannel var4) throws IOException {
int var5 = Math.min(var3, 8192);
ByteBuffer var6 = Util.getTemporaryDirectBuffer(var5);
```
var6 above is the 8KB buffer.
----------------------------------------------------------------
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 #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
Posted by GitBox <gi...@apache.org>.
runzhiwang commented on a change in pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296#discussion_r529465495
##########
File path: ratis-client/src/main/java/org/apache/ratis/client/impl/DataStreamClientImpl.java
##########
@@ -22,25 +22,39 @@
import org.apache.ratis.client.DataStreamOutputRpc;
import org.apache.ratis.conf.RaftProperties;
import org.apache.ratis.datastream.impl.DataStreamPacketByteBuffer;
+import org.apache.ratis.datastream.impl.DataStreamReplyByteBuffer;
import org.apache.ratis.proto.RaftProtos.DataStreamPacketHeaderProto.Type;
import org.apache.ratis.protocol.ClientId;
+import org.apache.ratis.protocol.ClientInvocationId;
import org.apache.ratis.protocol.DataStreamReply;
+import org.apache.ratis.protocol.RaftClientReply;
import org.apache.ratis.protocol.RaftClientRequest;
import org.apache.ratis.protocol.RaftGroupId;
import org.apache.ratis.protocol.RaftPeer;
import org.apache.ratis.protocol.exceptions.AlreadyClosedException;
+import org.apache.ratis.util.IOUtils;
import org.apache.ratis.util.JavaUtils;
import org.apache.ratis.util.MemoizedSupplier;
import java.io.IOException;
import java.nio.ByteBuffer;
+import java.nio.channels.WritableByteChannel;
import java.util.concurrent.CompletableFuture;
/**
* Streaming client implementation
* allows client to create streams and send asynchronously.
*/
public class DataStreamClientImpl implements DataStreamClient {
+ private static RaftClientReply getRaftClientReply(DataStreamReply reply) {
Review comment:
Duplicate with the following code.
```
static RaftClientReply getRaftClientReply(DataStreamReply dataStreamReply) {
if (dataStreamReply instanceof DataStreamReplyByteBuffer) {
try {
return ClientProtoUtils.toRaftClientReply(((DataStreamReplyByteBuffer) dataStreamReply).slice());
} catch (InvalidProtocolBufferException e) {
throw new IllegalStateException("Failed to parse " + JavaUtils.getClassSimpleName(dataStreamReply.getClass())
+ ": reply is " + dataStreamReply, e);
}
} else {
throw new IllegalStateException("Unexpected " + dataStreamReply.getClass() + ": reply is " + dataStreamReply);
}
}
```
----------------------------------------------------------------
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 #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
Posted by GitBox <gi...@apache.org>.
szetszwo merged pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296
----------------------------------------------------------------
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 #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
Posted by GitBox <gi...@apache.org>.
runzhiwang edited a comment on pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296#issuecomment-732968122
transferTo can avoid reading data to user space, it transfer data between kernel space and socket/disk.
1. But we still need to calculate checksum, so client still need read data to user space.
2. Besides, if we use transferTo, client transfer data to three nodes in parallel maybe best, otherwise if we use client -> primary -> peer, primary must wait finish write 128MB block, and then transfer the whole block file to follower, it maybe slow, but if client transfer data to three nodes in parallel, "the client has a higher network bandwidth requirement and it cannot optimise the network traffic".
So not sure whether can use transferTo in ozone.
----------------------------------------------------------------
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 pull request #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296#issuecomment-732963946
We still can use transferTo for file > 128MB since the API can specify position and count.
https://netty.io/4.1/api/io/netty/channel/DefaultFileRegion.html#DefaultFileRegion-java.nio.channels.FileChannel-long-long-
User may pass a large file. Internally, we send it by chunks.
----------------------------------------------------------------
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 #296: RATIS-1175. DataStreamOutput should support FileChannel#transferTo.
Posted by GitBox <gi...@apache.org>.
runzhiwang edited a comment on pull request #296:
URL: https://github.com/apache/incubator-ratis/pull/296#issuecomment-732968122
transferTo can avoid reading data to user space, it transfer data between kernel space and socket/disk.
1. But we need to calculate checksum, so client still need read data, to avoid reading data to user space, we can use MapByteBuffer. so we need 2 times read data from disk to kernel space: transferTo and MapByteBuffer.
2. Besides, if we use transferTo, client transfer data to three nodes in parallel maybe best, otherwise if we use client -> primary -> peer, primary must wait finish write 128MB block, and then transfer the whole block file to follower, it maybe slow, but if client transfer data to three nodes in parallel, "the client has a higher network bandwidth requirement and it cannot optimise the network traffic".
So not sure whether we can use transferTo in ozone.
----------------------------------------------------------------
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