Skip to content

HDDS-2424. Add the recover-trash command server side handling. #399

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
bshashikant merged 12 commits into from
May 5, 2020
Merged

HDDS-2424. Add the recover-trash command server side handling. #399

bshashikant merged 12 commits into from
May 5, 2020

Conversation

@cxorm
Copy link
Member

@cxorm cxorm commented Dec 28, 2019 •
edited
Loading

What changes were proposed in this pull request?

This PR was created for adding the server side of recover-trash.

Components updated including (in propagation order.)
OzoneManager and OzoneManagerRequestHandler
KeyManager and its implementation
OMMetadataManager and its implementation

Other fixes would be completed in HDDS-2425 and HDDS-2426
(Including startKey and prefix)

Note

Cause recoverTrash is write request, we should handle the request with OMHA.

With this doc, we use late validation to handle write request.

This PR mainly has parts including updating OzoneManager and
fixing OzoneManagerProtocolClientSideTranslatorPB as well as
OMTrashRecoverRequest and OMTrashRecoverResponse to handle request of write type.

What is the link to the Apache JIRA

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

How was this patch tested?

Cause only adding the server side handling.
Just tested the propagation and ran the UT.

maobaolong
maobaolong reviewed Dec 30, 2019
View reviewed changes
Copy link
Member

@maobaolong maobaolong left a comment

Choose a reason for hiding this comment

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

Look good overall, just some minor comments left.

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java Outdated
public boolean recoverTrash(String volumeName, String bucketName,
String keyName, String destinationBucket) throws IOException {

Preconditions.checkNotNull(volumeName);
Copy link
Member

@maobaolong maobaolong Dec 30, 2019

Choose a reason for hiding this comment

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

Not sure the @nonnull is a better way? I saw the Nonnull annotation in the hadoop-ozone project already, and findbugs will find null argument during build.

Copy link
Member Author

@cxorm cxorm Dec 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @maobaolong for this advice.

As I know, we could use both @Nonnull and Preconditions.checkNotNull(), and the former is to inform developer not use null in the parameter, and the later is to validate in runtime.

So, I think this part would not be fixed,
and we could create a Jira for the annotation issue. HDDS-2824

Copy link
Member

@maobaolong maobaolong Dec 30, 2019

Choose a reason for hiding this comment

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

ok

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java Outdated
public boolean recoverTrash(String volumeName, String bucketName,
String keyName, String destinationBucket) throws IOException {

// TODO: core logic stub would be added in later patch.
Copy link
Member

@maobaolong maobaolong Dec 30, 2019

Choose a reason for hiding this comment

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

Maybe we need a JIRA tickets track this item?

Copy link
Member Author

@cxorm cxorm Dec 30, 2019

Choose a reason for hiding this comment

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

Thanks @maobaolong for the suggestion.

Yeah, there are HDDS-2425 and HDDS-2426 that tracked this item.
Updated.

hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestTrashService.java
String destinationBucket = "destBucket";
createAndDeleteKey(keyName);

/* TODO:HDDS-2424. */
Copy link
Member

@maobaolong maobaolong Dec 30, 2019

Choose a reason for hiding this comment

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

Yeah, what i means is like this TODO comment.

@xiaoyuyao xiaoyuyao requested a review from anuengineer January 6, 2020 17:18
@bharatviswa504
Copy link
Contributor

bharatviswa504 commented Jan 6, 2020

Recover trash is a write request command as this moves the keys from the delete table to the original key table. let me know if I am missing something here.

2 comments.

  1. In code it is marked as read-only, should it be write request in OM?
  2. If write request should follow the implementation of write requests (preExecute/ValidateAndUpdatecache)

As for write request commands, we should follow HA kind of request implementation. For reference this design link write link

bharatviswa504
bharatviswa504 requested changes Jan 6, 2020
View reviewed changes
Copy link
Contributor

@bharatviswa504 bharatviswa504 left a comment

Choose a reason for hiding this comment

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

One General question I have on the PR approach.

@bharatviswa504
Copy link
Contributor

bharatviswa504 commented Jan 7, 2020 •
edited
Loading

@cxorm Attached the Cache Design document to HDDS-505. Sorry for the trouble the links in the document of write link above are internal links. So, attached them to HDDS-505

@cxorm
Copy link
Member Author

cxorm commented Jan 7, 2020

@cxorm Attached the Cache Design document to HDDS-505. Sorry for the trouble the links in the document of write link above are internal links. So, attached them to HDDS-505

Thank you @bharatviswa504 for the document.
I'm going to fix this.

@cxorm
Copy link
Member Author

cxorm commented Jan 19, 2020

Thanks @bharatviswa504 for the document.

The recoverTrash is type of write request, and with the document
I updated the PR with OMTrashRecoverRequest#preExecute and OMTrashRecoverRequest#validateAndUpdateCache as well as the draft of OMTrashRecoverResponse.
The fully implementation would be completed by following PR.

@cxorm cxorm requested a review from bharatviswa504 January 19, 2020 18:05
@elek
Copy link
Member

elek commented Feb 10, 2020

@bharatviswa504 Can we commit this PR?

bharatviswa504
bharatviswa504 reviewed Feb 14, 2020
View reviewed changes
...ne-manager/src/main/java/org/apache/hadoop/ozone/om/response/key/OMTrashRecoverResponse.java
omKeyInfo = OmUtils.prepareKeyForRecover(omKeyInfo, repeatedOmKeyInfo);
omMetadataManager.getDeletedTable()
.deleteWithBatch(batchOperation, omKeyInfo.getKeyName());
/* TODO: trashKey should be updated to destinationBucket. */
Copy link
Contributor

@bharatviswa504 bharatviswa504 Feb 14, 2020

Choose a reason for hiding this comment

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

One question:

  1. if the key is created, deleted, the key is created and the key is deleted. Now when recover, which omKeyInfo will be used from the delete table.

Copy link
Member Author

@cxorm cxorm Mar 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you @bharatviswa504 for the question.

Refer to processing of DeletedTable in OMKeyDeleteResponse#addToDBBatch() and OmUtils#prepareKeyForDelete(), the latest deleted key is added in tail of RepeatedOmKeyInfo#omKeyInfoList.

So I think we could recover the latest deleted key from the DeletedTable in this created-deleted-created-deleted situation. (And when recovering the latest key, I think we should clear the old deleted key.)

Would you please give me advice if I miss something ?
If the idea is proper, I will update the description of this jira.

Copy link
Contributor

@bharatviswa504 bharatviswa504 Mar 26, 2020

Choose a reason for hiding this comment

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

I am fine with recovering last delete key if that is the expected behavior.

(And when recovering the latest key, I think we should clear the old deleted key.)
We should not delete the other keys, as those keys will be picked by background trash service and the data for those keys need to be deleted.

And also doing this way, is also not correct from my understanding, let us say, we put those keys in delete table, and background delete key service will pick them up and send to SCM for deletion, at this point we got a recover trash command, so there is a chance that we recover the key which might have no data, as we submitted the request to SCM for deletion, and SCM, in turn, it will send to DN. How we shall handle this kind of scenarios?

Because deletion from delete table will happen when key purge request happens.

Code snippet link #link

Copy link
Member Author

@cxorm cxorm Apr 3, 2020

Choose a reason for hiding this comment

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

/pending I'm tracing the background part. (Hope soon)

Copy link
Member Author

@cxorm cxorm Apr 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you @bharatviswa504 for taking time to review this.

Here is my thought,
We set modificationTime when deleting key.

So I think we can compare the modificationTime with RECOVERY_WINDOW to exclude keys(exist in trash-enabled buckets) from purging.

Code snippet would be added after this line might like

if (trashEnable(info.getBucketName()) &&
    (Time.now() - info.getModificationTime()) < RECOVERY_WINDOW) {
    
  /* Would not delete key in this situation. */
}

note recovery_window of bucket would be added in later Jira.

Could you please give me your thoughts or ideas if I miss something, thank you.

And here is discussion about trash-recovery.

bharatviswa504
bharatviswa504 reviewed Feb 14, 2020
View reviewed changes
Copy link
Contributor

@bharatviswa504 bharatviswa504 left a comment

Choose a reason for hiding this comment

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

Few comments inline.
Thank You @cxorm for the update.

@elek
Copy link
Member

elek commented Mar 10, 2020

/pending Comments from @bharatviswa504 are not addressed, yet...

github-actions[bot]
github-actions bot requested changes Mar 10, 2020
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Marking this issue as un-mergeable as requested.

Please use /ready comment when it's resolved.

Comments from @bharatviswa504 are not addressed, yet...

@cxorm cxorm requested a review from bharatviswa504 March 25, 2020 15:51
@elek
Copy link
Member

elek commented Apr 1, 2020

@bharatviswa504 Can you please check it?

@bharatviswa504
Copy link
Contributor

bharatviswa504 commented Apr 6, 2020

@elek There are some pending comments which need to be resolved.

@cxorm cxorm force-pushed the HDDS-2424 branch 4 times, most recently from 749714e to 6c0fa96 Compare April 12, 2020 21:42
cxorm
cxorm commented Apr 12, 2020
View reviewed changes
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocol/OzoneManagerProtocol.java Outdated
Comment on lines +555 to +509
Copy link
Member Author

@cxorm cxorm Apr 12, 2020

Choose a reason for hiding this comment

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

@bharatviswa504
What do you think about this cleanup write-request ?
Could we set the write-operation with a default or
we need a separated interface addressed the write-operation ?

@bshashikant
Copy link
Contributor

bshashikant commented Apr 17, 2020

@cxorm , can you plz update the pr?

@bshashikant
Copy link
Contributor

bshashikant commented Apr 24, 2020

@bharatviswa504 , can you please have a look?

@cxorm
Copy link
Member Author

cxorm commented Apr 29, 2020

Thanks @bshashikant for the reminder.
Rebase latest master-branch (#843).

@cxorm
Copy link
Member Author

cxorm commented Apr 30, 2020

Rebase latest master-branch (#839) to resolve conflict.

@cxorm
Copy link
Member Author

cxorm commented May 5, 2020

Rebase latest master-branch (#848) and trigger github-actions.

@bshashikant bshashikant merged commit f20cb4e into apache:master May 5, 2020
@bshashikant
Copy link
Contributor

bshashikant commented May 5, 2020

Thanks @cxorm for working on this. I have committed this.

@cku328 cku328 mentioned this pull request May 5, 2020
Merged
@cxorm
Copy link
Member Author

cxorm commented May 6, 2020

Thanks @bharatviswa504 for the review
and thanks @bshashikant for the commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maobaolong maobaolong maobaolong left review comments

@github-actions github-actions[bot] github-actions[bot] requested changes

@bshashikant bshashikant bshashikant approved these changes

@anuengineer anuengineer Awaiting requested review from anuengineer

@bharatviswa504 bharatviswa504 Awaiting requested review from bharatviswa504

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@cxorm @bharatviswa504 @elek @bshashikant @maobaolong