Uh oh!
There was an error while loading. Please reload this page.
HDDS-5599. [Ozone-Streaming]drop BufferPool and ChunkBuffer to avoid buffer copying - #2557
Conversation
Hi @szetszwo@bshashikant@mukul1987 |
d46f6fc to
f4e4210Compare
szetszwo
left a comment
There was a problem hiding this comment.
The change looks good. Some comments inlined. Thanks.
| // TODO We are no longer using the bufferPool, so we do not need to flush the | ||
| // data in it. We can discuss whether add other behaviors here. |
There was a problem hiding this comment.
flush() should wait for all the futures to complete. Move the code below
//executePutBlock(..)
try {
CompletableFuture.allOf(futures.toArray(EMPTY_FUTURE_ARRAY)).get();
} catch (Exception e) {
LOG.warn("Failed to write all chunks through stream: " + e);
throw new IOException(e);
}
from executePutBlock(..) to flush().
| } else { | ||
| byteBufferList = null; | ||
| } | ||
There was a problem hiding this comment.
Move the try-catch below to flush() and then call flush() here.
| futureMap = new ConcurrentHashMap<>(); | ||
| } | ||
| public CommitWatcher(XceiverClientSpi xceiverClient) { |
There was a problem hiding this comment.
Let's copy CommitWatcher to a new class, say StreamCommitWatcher? In StreamCommitWatcher, we don't need bufferPool. The commitIndex2flushedDataMap should be different -- it should not use ChunkBuffer anymore.
5b377ae to
4a71962Compareszetszwo
left a comment
There was a problem hiding this comment.
+1, the change looks good.
There are some test failures. Could you take a look?
captainzmc
commented
Aug 25, 2021
CI runs correctly on my personal branch. It seems the error is not caused by this PR. I will try to re-trigger the CI to make sure this. |
captainzmc
commented
Aug 25, 2021
hi @szetszwo, CI has been running correct. Could you help take another look this PR? |
kaijchen
commented
Aug 26, 2021
We no longer copy the data into buffer pool. But we still need to retain the ByteBuf until it gets flushed. This retain is for the retry. |
szetszwo
commented
Aug 26, 2021
@bshashikant , @kaijchen , @captainzmc , I suggest that we first make the streaming write working (without retry, checksum, error handling, etc) so that we have something to test. Then, we can add those features. What do you think? |
Agree with @szetszwo. |
…buffer copying (apache#2557) (cherry picked from commit db89077)
What changes were proposed in this pull request?
In BlockDataStreamOutput, we should write directly to the underlying stream. We should not use BufferPool and ChunkBuffer in BlockDataStreamOutput anymore since we want to avoid buffer copying.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5599
How was this patch tested?
Existing UT