Skip to content

HDDS-6543. [Merge rocksdb in datanode] BlockDeletingService adaptation for schema v3 containers.#3385

Merged
ChenSammi merged 4 commits intoapache:HDDS-3630from
guihecheng:HDDS-6543
May 9, 2022
Merged

HDDS-6543. [Merge rocksdb in datanode] BlockDeletingService adaptation for schema v3 containers.#3385
ChenSammi merged 4 commits intoapache:HDDS-3630from
guihecheng:HDDS-6543

Conversation

@guihecheng
Copy link
Contributor

What changes were proposed in this pull request?

BlockDeletingService adaptation for schema v3 containers.

What is the link to the Apache JIRA

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

How was this patch tested?

turned on previously ignored UT for schema v3.

@guihecheng
Copy link
Contributor Author

cc @ChenSammi

@guihecheng guihecheng changed the title HDDS-6543. BlockDeletingService adaptation for schema v3 containers. HDDS-6543. [Merge rocksdb in datanode] BlockDeletingService adaptation for schema v3 containers. May 6, 2022
}

@SuppressWarnings("checkstyle:parameternumber")
private void createPendingDeleteBlocksSchema3(int numOfBlocksPerContainer,
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like createPendingDeleteBlocksSchema3 and createPendingDeleteBlocksSchema2 are identical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is, here I just intend to keep the schema check style of function in createToDeleteBlocks aligned, but I could remove one of the identical functions and use the lower level function directly when we have schema v2 || v3.

private interface Deleter {
void apply(Table<?, DeletedBlocksTransaction> deleteTxnsTable,
BatchOperation batch, long txnID)
throws IOException;
Copy link
Contributor

Choose a reason for hiding this comment

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

L553 and L553 can be merged into one line.

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, I'll merge it.

*/
public class DatanodeStoreSchemaThreeImpl extends AbstractDatanodeStore {
public class DatanodeStoreSchemaThreeImpl extends AbstractDatanodeStore
implements DeleteTransactionStore<String> {
Copy link
Contributor

Choose a reason for hiding this comment

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

"Rename dropAllWithPrefix to removeContainerFromDB". Change is missed from last patch.

Copy link
Contributor Author

@guihecheng guihecheng May 9, 2022

Choose a reason for hiding this comment

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

Ah, I think I just renamed the helper function in BlockUtils but not the one in DatanodeStoreSchemaThreeImpl.
I think we could use a slightly different name like removeKVContainerData, because I found a existing name for those KV pairs for container: KeyValueContainerUtil.parseKVContainerData().
And I'll add dumpKVContainerDataXXX and loadKVContainerDataXXX in the next patch for container export and import.

@ChenSammi
Copy link
Contributor

Thanks @guihecheng, the patch overall looks good.

@ChenSammi
Copy link
Contributor

+1.

@ChenSammi ChenSammi merged commit 562a242 into apache:HDDS-3630 May 9, 2022
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.

2 participants