Skip to content

Conversation

@runzhiwang
Copy link
Contributor

What changes were proposed in this pull request?

DeleteBlock needs to add/update/delete DELETED_BLOCKS_TABLE in SCMMetadataDB. So SCM does these operations via ratis.

What is the link to the Apache JIRA

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

How was this patch tested?

Existed UT.

@runzhiwang
Copy link
Contributor Author

@GlenGeng @amaliujia Could you help review this ? Thanks a lot.

@amaliujia
Copy link
Contributor

Will take a look! Thanks for this PR.

@runzhiwang runzhiwang changed the title HDDS-3205. Handle BlockDeletingService in SCM HA HDDS-3205. DeleteBlock via Ratis in SCM HA Jan 12, 2021
@runzhiwang runzhiwang closed this Jan 13, 2021
@runzhiwang runzhiwang reopened this Jan 13, 2021
Copy link
Contributor

@GlenGeng-awx GlenGeng-awx left a comment

Choose a reason for hiding this comment

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

LGTM. left some inline comments. Thanks for the work!

@runzhiwang runzhiwang force-pushed the HDDS-2823-delete-block branch from 31daaec to 565326f Compare January 15, 2021 08:26
@runzhiwang runzhiwang force-pushed the HDDS-2823-delete-block branch from 565326f to 9de897b Compare January 22, 2021 01:34
@runzhiwang
Copy link
Contributor Author

@GlenGeng @bshashikant @amaliujia Thanks for review. I have updated the patch.

@GlenGeng-awx
Copy link
Contributor

GlenGeng-awx commented Jan 24, 2021

@runzhiwang Is the CI failure related to your changes ?

They seems to be affected by your changes.

TestStorageContainerManager.testBlockDeletingThrottling:382
TestStorageContainerManager.testBlockDeletionTransactions:290
TestDeleteWithSlowFollower.testDeleteKeyWithSlowFollower:282 ? Timeout Timed o…

@runzhiwang runzhiwang closed this Jan 26, 2021
@runzhiwang runzhiwang reopened this Jan 26, 2021
Copy link
Contributor

@GlenGeng-awx GlenGeng-awx left a comment

Choose a reason for hiding this comment

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

LGTM, some typo related comments.

@runzhiwang runzhiwang force-pushed the HDDS-2823-delete-block branch 2 times, most recently from d62e6cc to 744f6e3 Compare January 27, 2021 08:59
@GlenGeng-awx
Copy link
Contributor

LGTM.

@bshashikant @amaliujia @nandakumar131 Please take one more look at this PR. Thanks!

This PR has implemented all the functionality according to the design: https://docs.google.com/document/d/166Aea2EowSGWtAFWNlDv0gu4rA06dQ2rJAsBd-l210Q/edit?usp=sharing.

After it is merged. all the features of Phase 2.0 of SCM HA will be finished.

@amaliujia
Copy link
Contributor

LGTM

@GlenGeng-awx
Copy link
Contributor

@runzhiwang Please rebase the HDDS-2823, since SCMServiceManager is merged.

@runzhiwang runzhiwang force-pushed the HDDS-2823-delete-block branch from 744f6e3 to 928a9ce Compare January 28, 2021 02:56
@runzhiwang
Copy link
Contributor Author

@GlenGeng I have rebased it.

@bshashikant bshashikant merged commit b328608 into apache:HDDS-2823 Jan 28, 2021
@bshashikant
Copy link
Contributor

Thanks @runzhiwang for the contribution.

@linyiqun
Copy link
Contributor

Sorry for the late comments.

I have one concern about current SCM HA handling mechanism. As the Blocks/Container/Pipeline update ops are cached in BatchOperation, and only be flushed during SCM snapshot take. But if user use the stand alone SCM mode to run based on current HA code. Is that will lead OOM error since stand alone SCM won't take any snapshot by itself while the request update will still go into SCM Transaction Buffer in our current implementation.

@runzhiwang @GlenGeng , is this a real problem of my concern?

@GlenGeng-awx
Copy link
Contributor

Hey @linyiqun, thanks for the review !

We planed to use single server raft cluster for the stand alone mode, which means the taking snapshot functionality works too.

@linyiqun
Copy link
Contributor

We planed to use single server raft cluster for the stand alone mode, which means the taking snapshot functionality works too.

Okay, after that, that's not a problem.

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.

6 participants