Skip to content

Conversation

@szetszwo
Copy link
Contributor

What changes were proposed in this pull request?

ReadChunk and GetSmallFile currently do not really require a DispatcherContext. So HDDS-9697 set a constant context for them.

In this pull request, we change it to creating a new DispatcherContext for ReadChunk and GetSmallFile. It is required by HDDS-9536 for storing the read buffers in order to release the buffers after the operation has completed.

What is the link to the Apache JIRA

HDDS-9936

How was this patch tested?

Updating some tests.

Copy link
Contributor

@duongkame duongkame left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch @szetszwo . I put a comment inline below.

if (popStream != null) {
IOUtils.close(LOG, popStream);
}
if (context != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar context creation/close should be done for ContainerStateMachine#readStateMachineData?

Copy link
Contributor Author

@szetszwo szetszwo Dec 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@duongkame , Good catch! We should if we make DispatcherContext closable for releasing buffers.

On a second thought, HDDS-7117 already has changed using MappedByteBuffer instead of allocating buffers when the size is larger than ozone.chunk.read.mapped.buffer.threshold (default 32KB). So, it seems that we don't have do anything for read. Could you test HDDS-9536 with the current code?

(Revised: fixed typo: "small" -> "larger")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds nice. @szetszwo , will update.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems that we don't have do anything for read

@szetszwo Should close this PR and resolve HDDS-9936 then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, let's close this.

@szetszwo szetszwo marked this pull request as draft December 17, 2023 03:58
@szetszwo szetszwo closed this Apr 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants