Skip to content

HDDS-9915. [hsync] Interface to retrieve block info and finalize block in DN through ratis. #5783

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
merged 10 commits into from
Dec 27, 2023
Merged

HDDS-9915. [hsync] Interface to retrieve block info and finalize block in DN through ratis. #5783

merged 10 commits into from
Dec 27, 2023

Conversation

ashishkumar50
Copy link
Contributor

What changes were proposed in this pull request?

New interface in datanode to finalize block and return block info.
If the block is already finalized just return back the block info. Keep block info in memory for which there is a finalize request, so that no new WRITE CHUNK/PUT Block is accepted for that block.
Persist finalize block ID into new rocksdb column family.
Clear the memory and delete from rocksdb during container Close.

What is the link to the Apache JIRA

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

How was this patch tested?

Unit and Integration test

@ashishkumar50 ashishkumar50 marked this pull request as draft December 13, 2023 13:31
@ashishkumar50
Copy link
Contributor Author

@ChenSammi @jojochuang, Please help to review.

@ashishkumar50 ashishkumar50 changed the title HDDS-9915. hsync: Interface to retrieve block info and finalize block in DN through ratis. HDDS-9915. [hsync] Interface to retrieve block info and finalize block in DN through ratis. Dec 13, 2023
jojochuang
jojochuang reviewed Dec 16, 2023
View reviewed changes
Copy link
Contributor

@jojochuang jojochuang 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 PR! Please check out my comments:

...a/org/apache/hadoop/ozone/container/common/transport/server/ratis/ContainerStateMachine.java
@@ -376,8 +377,20 @@ public TransactionContext startTransaction(RaftClientRequest request)
ctxt.setException(ioe);
return ctxt;
}
if (proto.getCmdType() == Type.WriteChunk) {
if (proto.getCmdType() == Type.PutBlock) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does PutBlock need change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to reject request from original client if there is any PUT block request after finalize block is called by recoverer client.

...-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerData.java Outdated
@@ -384,6 +413,10 @@ public String getDeletingBlockKeyPrefix() {
return formatKey(DELETING_KEY_PREFIX);
}

public String getFinalizeBlockKey() {
return formatKey("");
Copy link
Contributor

Choose a reason for hiding this comment

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

missing a key here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to keep only containerId as key, formatKey returns key as containerId in v3, so that would work.
But because of this comment, changed now key format to use containerId|localID

.../src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java Outdated
store.getFinalizeBlocksTable();

FinalizeBlockList finalizeBlockList =
finalizeBlocksTable.get(kvContainerData.getFinalizeBlockKey());
Copy link
Contributor

Choose a reason for hiding this comment

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

Here it looks a little strange. Assuming you fetch FinalizeBlockList regardless of their containers, you would get all finalize blocks, but FinalizeBlockList is a list of Long, it is unable to tell what containers the blocks belong too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect you want to use KeyValueContainerData.getBlockKey() to add prefix for different containers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would have loaded only container specific list as based on previous comment format key returns containerId as key in schema v3. Now i have changed key structure as previous comment..

ChenSammi
ChenSammi reviewed Dec 18, 2023
View reviewed changes
...a/org/apache/hadoop/ozone/container/common/transport/server/ratis/ContainerStateMachine.java
} else {
return TransactionContext.newBuilder()
} else if (proto.getCmdType() == Type.FinalizeBlock) {
containerController.addFinalizedBlock(proto.getContainerID(),
Copy link
Contributor

Choose a reason for hiding this comment

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

We cannot add the finalize block in startTransaction, otherwise there is chance that valid write chunk or put block in applyTransaction will be failed because of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are rejecting request only in startTransaction and not in applyTransaction. So once the request has passed in startTransaction I think it should succeed in applyTransaction. In which case this can happen?

Copy link
Contributor

@ChenSammi ChenSammi Dec 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I see. Currently finalized block is not checked in applyTransaction. Then it's fine.

ChenSammi
ChenSammi reviewed Dec 18, 2023
View reviewed changes
...rc/main/java/org/apache/hadoop/ozone/container/metadata/DatanodeSchemaThreeDBDefinition.java
FixedLengthStringCodec.get(),
FinalizeBlockList.class,
FinalizeBlockList.getCodec());

Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't use list as value here. The list will have performance impact. For example, if there is 99 blocks already, then the 100th block comes, we need to persist the whole list with 100 elements in to RocksDB, the list introduces unnecessary bytes to write.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed storage as <ContainerId | LocalId, BlockData>. Since DatanodeTable iterator doesn't support. I store value as BlockData and currently used keyValueBlockIterator which can iterate through blocks. Is this fine to store or do you have any other suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

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

How about implement a new implementation of BlockIterator? Since only the local ID is useful, then the value can be just the local ID.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added new implementation of BlockIterator and store value as localId only.

@ChenSammi ChenSammi marked this pull request as ready for review December 25, 2023 07:21
@ChenSammi ChenSammi force-pushed the HDDS-9915 branch 2 times, most recently from 27a9d9f to a26fbb6 Compare December 26, 2023 14:48
ChenSammi
ChenSammi approved these changes Dec 27, 2023
View reviewed changes
Copy link
Contributor

@ChenSammi ChenSammi left a comment

Choose a reason for hiding this comment

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

Thanks @ashishkumar50 for the contribution, and @jojochuang for the review.

@ChenSammi ChenSammi merged commit 840900f into apache:HDDS-7593 Dec 27, 2023
jojochuang pushed a commit to jojochuang/ozone that referenced this pull request Jan 17, 2024
@ashishkumar50 @jojochuang
HDDS-9915. [hsync] Interface to retrieve block info and finalize bloc…
15d803b 
…k in DN through ratis. (apache#5783)
@jojochuang jojochuang added the hbase HBase on Ozone support label Jan 23, 2024
chungen0126 pushed a commit to chungen0126/ozone that referenced this pull request May 3, 2024
@ashishkumar50 @chungen0126
HDDS-9915. [hsync] Interface to retrieve block info and finalize bloc…
6e39499 
…k in DN through ratis. (apache#5783)
chungen0126 pushed a commit to chungen0126/ozone that referenced this pull request May 3, 2024
@ashishkumar50 @chungen0126
HDDS-9915. [hsync] Interface to retrieve block info and finalize bloc…
ae6ec0c 
…k in DN through ratis. (apache#5783)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jojochuang jojochuang jojochuang left review comments

@ChenSammi ChenSammi ChenSammi approved these changes

Assignees
No one assigned
Labels
hbase HBase on Ozone support
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ashishkumar50 @jojochuang @ChenSammi