HDDS-4553. ChunkInputStream should release buffer as soon as last byte in the buffer is read #2062
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…e in the buffer is read
Contributor
Author
|
@adoroszlai, I fixed the review comments you posted in HDDS-4552 here. Please take a look. Thanks. |
Contributor
| int dataLength = CHUNK_SIZE; | ||
| byte[] inputData = writeRandomBytes(keyName, dataLength); | ||
|
|
||
| KeyInputStream keyInputStream = getKeyInputStream(keyName); |
Contributor
There was a problem hiding this comment.
You may want to use try(KeyInputStream keyInputStream = getKeyInputStream(keyName)){... }
So, that stream will be autoclosed
| private long bufferOffsetWrtChunkData; | ||
|
|
||
| // Index of the first buffer which has not been released | ||
| private int minBufferIndex = 0; |
Contributor
There was a problem hiding this comment.
I am little bit confused with this. Can we think better name? unreleasedFirstBuffIdx or so ?
|
|
||
|
|
||
| /** | ||
| * Release a buffer. |
Contributor
There was a problem hiding this comment.
Probably we can refine this doc a bit? This method releases "buffers" till the given index position .
| // no-op | ||
| } | ||
|
|
||
| default long getBufferCapacityForChunkRead(ChunkInfo chunkInfo, |
Contributor
There was a problem hiding this comment.
this can be static method right?
Contributor
|
@hanishakoneru Thank you for the patch. I have posted few minor comments. Please check when you get time. Thanks |
mukul1987
force-pushed
the
master
branch
2 times, most recently
from
79a9d39 to
520ba00
Compare
Contributor
Author
|
Thank you @umamaheswararao for the review. I have addressed your comments in the latest commit. |
umamaheswararao
approved these changes
Mar 31, 2021
|
|
||
|
|
||
| /** | ||
| * Release a buffers upto the given index. |
Contributor
There was a problem hiding this comment.
Tiny nit: make sure to change it before commit
Release a buffers --> Release the buffers
bshashikant
approved these changes
Apr 1, 2021
Contributor
bshashikant
left a comment
bshashikant
left a comment
There was a problem hiding this comment.
Looks good. Thanks @hanishakoneru for working on this.
Contributor
Author
|
Thank you @umamaheswararao and @bshashikant for the reviews. |
errose28
added a commit
to errose28/ozone
that referenced
this pull request
Apr 7, 2021
* HDDS-3698-nonrolling-upgrade: (144 commits) fix project name in NOTICE.txt (apache#2112) HDDS-5066. Use fixed vesion from pnpm to build recon (apache#2115) HDDS-5014. Add non-rolling upgrade design docs. HDDS-5035. Use default config values to solve generated config file conflict (apache#2087) HDDS-5032. DN stopped to load containers on volume after a container load exception. (apache#2109) HDDS-4504. Datanode deletion config should be based on number of blocks (apache#1885) Fix ozone-ha acceptance test. HDDS-5058. Make getScmInfo retry for a duration. HDDS-4506. Support query parameter based v4 auth in S3g (apache#1628) HDDS-4553. ChunkInputStream should release buffer as soon as last byte in the buffer is read (apache#2062) HDDS-5022. SCM get roles command should provide Ratis Leader/Follower… (apache#2098) HDDS-5033. SCM may not be able to know full port list of Datanode after Datanode is started. (apache#2090) HDDS-3752. Fix o3fs list bucket contents issue when without tailing "/" (apache#2088) HDDS-4901. Remove OmOzoneAclMap from OmVolumeArgs to avoid OzoneAcl conversions (apache#1992) HDDS-4987. Import container should not delete container contents if container already exists (apache#2077) Checkstyle fix. Intialize DN layout version before security init. HDDS-4915. [SCM HA Security] Integrate CertClient. (apache#2000) HDDS-5049. Add timeout support for ratis requests in SCM HA. (apache#2099) trigger new CI check ...
errose28
added a commit
to errose28/ozone
that referenced
this pull request
Apr 9, 2021
* HDDS-3698-nonrolling-upgrade: (150 commits) HDDS-5056. Avoid false positiver error messages during pipeline creations (apache#2105) HDDS-5027. [SCM HA Security] Handle leader changes during bootstrap. (apache#2113) HDDS-5032. Fix findbugs (apache#2120) HDDS-5062. Add a config to bypass clusterId validation for bootstrapping SCM. (apache#2114) HDDS-5011. Introduce Java based ReplicationConfig implementation (apache#2089) HDDS-4925. Introduce ContainerBalancer in SCM with start/stop capabilities. (apache#2097) fix project name in NOTICE.txt (apache#2112) HDDS-5066. Use fixed vesion from pnpm to build recon (apache#2115) HDDS-5014. Add non-rolling upgrade design docs. HDDS-5035. Use default config values to solve generated config file conflict (apache#2087) HDDS-5032. DN stopped to load containers on volume after a container load exception. (apache#2109) HDDS-4504. Datanode deletion config should be based on number of blocks (apache#1885) Fix ozone-ha acceptance test. HDDS-5058. Make getScmInfo retry for a duration. HDDS-4506. Support query parameter based v4 auth in S3g (apache#1628) HDDS-4553. ChunkInputStream should release buffer as soon as last byte in the buffer is read (apache#2062) HDDS-5022. SCM get roles command should provide Ratis Leader/Follower… (apache#2098) HDDS-5033. SCM may not be able to know full port list of Datanode after Datanode is started. (apache#2090) HDDS-3752. Fix o3fs list bucket contents issue when without tailing "/" (apache#2088) HDDS-4901. Remove OmOzoneAclMap from OmVolumeArgs to avoid OzoneAcl conversions (apache#1992) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
We currently wait for reading up till the Chunk EOF before releasing the buffers in ChunkInputStream (or when the stream is closed). Let's say a client reads first 3 MB of a chunk of size 4MB and does not close the stream immediately. This would lead to the 3MB of data being cached in the ChunkInputStream buffers till the stream is closed.
After HDDS-4552, a chunk read from DN would return the chunk data as an array of ByteBuffers. After each ByteBuffer is read, it can be released. This would greatly help with optimizing memory usage of ChunkInputStream.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4553
How was this patch tested?
Added unit tests