Skip to content

HDDS-9751. [hsync] Make Putblock performance acceptable - DataNode side #5662

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
jojochuang merged 2 commits into from
Jan 23, 2024
Merged

HDDS-9751. [hsync] Make Putblock performance acceptable - DataNode side #5662

jojochuang merged 2 commits into from
Jan 23, 2024

Conversation

@jojochuang
Copy link
Contributor

@jojochuang jojochuang commented Nov 22, 2023

What changes were proposed in this pull request?

Implement DataNode side change for HDDS-8047 incremental chunk list for PutBlock. See HDDS-8047 for the problem description and design doc.

Please describe your PR in detail:

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-9751

How was this patch tested?

  • New unit tests in TestBlockManagerImpl
  • Full cluster HBase ycsb workload test

@jojochuang jojochuang requested a review from ChenSammi November 22, 2023 20:08
@jojochuang jojochuang mentioned this pull request Nov 22, 2023
Merged
@jojochuang jojochuang force-pushed the HDDS-9751 branch 3 times, most recently from f90b557 to f9db199 Compare December 21, 2023 19:06
@jojochuang jojochuang force-pushed the HDDS-9751 branch 4 times, most recently from 5e2b3c0 to 51247a2 Compare January 5, 2024 04:42
@jojochuang jojochuang marked this pull request as ready for review January 5, 2024 19:03
ashishkumar50
ashishkumar50 reviewed Jan 10, 2024
View reviewed changes
Copy link
Contributor

@ashishkumar50 ashishkumar50 left a comment

Choose a reason for hiding this comment

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

@jojochuang Thanks for the patch, Please find my comments.

...e/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeStoreSchemaThreeImpl.java Outdated
String blockKey = containerData.getBlockKey(blockID.getLocalID());

// check last chunk table
BlockData lastChunk = getLastChunkInfoTable().get(blockKey);
Copy link
Contributor

@ashishkumar50 ashishkumar50 Jan 10, 2024

Choose a reason for hiding this comment

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

If incremental is disabled we can avoid calling getLastChunkInfoTable().

Copy link
Contributor Author

@jojochuang jojochuang Jan 17, 2024

Choose a reason for hiding this comment

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

good idea.

...e/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeStoreSchemaThreeImpl.java Outdated
// update the last partial chunk
data.setChunks(lastChunkInfo);
getLastChunkInfoTable().putWithBatch(
batch, containerData.getBlockKey(localID), data);
Copy link
Contributor

@ashishkumar50 ashishkumar50 Jan 10, 2024

Choose a reason for hiding this comment

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

Can we just define and store the last chunk instead of blockData containing last chunk.

Copy link
Contributor Author

@jojochuang jojochuang Jan 17, 2024

Choose a reason for hiding this comment

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

i think there could still be cases where the client's buffer has more than one chunk's full of data that gets flushed out at a time.

@adoroszlai adoroszlai mentioned this pull request Jan 14, 2024
Closed
ChenSammi
ChenSammi reviewed Jan 16, 2024
View reviewed changes
...vice/src/test/java/org/apache/hadoop/ozone/container/keyvalue/impl/TestBlockManagerImpl.java Outdated

@Test
public void testFlush1() throws Exception {
assumeTrue(isSameSchemaVersion(schemaVersion, OzoneConsts.SCHEMA_V3));
Copy link
Contributor

@ChenSammi ChenSammi Jan 16, 2024

Choose a reason for hiding this comment

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

Since SCHEMA_V2 is also supported. We can use parameterized test to test both SCHEMA_V3 and SCHEMA_V2.

Copy link
Contributor Author

@jojochuang jojochuang Jan 17, 2024

Choose a reason for hiding this comment

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

do it in another way: skip if schema is v1.

ChenSammi
ChenSammi reviewed Jan 16, 2024
View reviewed changes
...e/src/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeStoreSchemaThreeImpl.java Outdated
// if eob or if the last chunk is full,
private static boolean shouldAppendLastChunk(boolean endOfBlock,
BlockData data) {
if (endOfBlock || data.getChunks().isEmpty()) {
Copy link
Contributor

@ChenSammi ChenSammi Jan 16, 2024

Choose a reason for hiding this comment

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

In what case that the data.getChunks will be empty?

Copy link
Contributor Author

@jojochuang jojochuang Jan 17, 2024

Choose a reason for hiding this comment

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

hsync right after creating a file might cause client to send empty chunk

Copy link
Contributor

@ChenSammi ChenSammi Jan 22, 2024

Choose a reason for hiding this comment

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

I see.

@ChenSammi
Copy link
Contributor

ChenSammi commented Jan 16, 2024

@jojochuang , for other cases like block deletion, container deletion, and container replica, will you file other JIRAs to support the incremental list table handling?

@jojochuang
Copy link
Contributor Author

jojochuang commented Jan 17, 2024

Yes HDDS-9753 is the catch-all for handling incremental chunk lists in other operations.

@jojochuang
HDDS-9751. [hsync] Make Putblock performance acceptable - DataNode side.
ea6247c 
Change-Id: Ie9201f2ddc288320fde6f8931af1edc3dd220bb7

Mock helper methods.

Change-Id: I07399ff1f18a9a3a1d5e147e0a8b57cf7d3dd949

Add Mock storage

Change-Id: Ib7413af4e531e5e083292a7a2ab08826fc4ba64a

Fix compilation error

Checkstyle

Fix

(cherry picked from commit 51247a2)

Address review comments.

Review comments.

Address Ashish's comments.
ChenSammi
ChenSammi reviewed Jan 22, 2024
View reviewed changes
...vice/src/test/java/org/apache/hadoop/ozone/container/keyvalue/impl/TestBlockManagerImpl.java
return blockData;
}

@ContainerTestVersionInfo.ContainerTest
Copy link
Contributor

@ChenSammi ChenSammi Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

When using @ContainerTestVersionInfo.ContainerTest, the test shall have a "ContainerTestVersionInfo versionInfo" parameter, otherwise, the parameterized test is not actually enabled.

@ContainerTestVersionInfo.ContainerTest public void testPendingDeleteBlockReset(ContainerTestVersionInfo versionInfo)

Copy link
Contributor Author

@jojochuang jojochuang Jan 23, 2024

Choose a reason for hiding this comment

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

Great catch! And I actually realized I had to move the code to a shared class so that both v2 and v3 can support this feature.

@jojochuang
Support both scehma v2 and v3.
0616da0 
Added another middle layer to share the code common to schema v2 and schema v3.
@jojochuang jojochuang requested a review from ChenSammi January 23, 2024 09:01
ChenSammi
ChenSammi approved these changes Jan 23, 2024
View reviewed changes
Copy link
Contributor

@ChenSammi ChenSammi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The last patch looks good. Thanks @jojochuang

@jojochuang jojochuang merged commit 305a176 into apache:HDDS-7593 Jan 23, 2024
@jojochuang
Copy link
Contributor Author

jojochuang commented Jan 23, 2024

Thanks @ChenSammi for the review!

@jojochuang jojochuang added the hbase HBase on Ozone support label Jan 23, 2024
@jojochuang jojochuang mentioned this pull request Mar 5, 2024
Merged
chungen0126 pushed a commit to chungen0126/ozone that referenced this pull request May 3, 2024
@jojochuang @chungen0126
HDDS-9751. [hsync] Make Putblock performance acceptable - DataNode si…
6c4c264 
…de (apache#5662)
chungen0126 pushed a commit to chungen0126/ozone that referenced this pull request May 3, 2024
@jojochuang @chungen0126
HDDS-9751. [hsync] Make Putblock performance acceptable - DataNode si…
846e927 
…de (apache#5662)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ashishkumar50 ashishkumar50 ashishkumar50 left review comments

@ChenSammi ChenSammi ChenSammi approved these changes

@szetszwo szetszwo Awaiting requested review from szetszwo

@duongkame duongkame Awaiting requested review from duongkame

Assignees

No one assigned

Labels

hbase HBase on Ozone support performance

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jojochuang @ChenSammi @ashishkumar50 @adoroszlai