Skip to content

Conversation

@cxorm
Copy link
Member

@cxorm cxorm commented May 22, 2020

What changes were proposed in this pull request?

This PR composed of changes including:

  • provide trash-enabled config when creating buckets.
  • provide recover-window config when creating buckets.
  • global config of above settings.
  • core logic of recovering trash to existing buckets.

Proposed fix.

  • In OmMetadataManagerImpl#getPendingDeletionKeys we only check last item in repeatedOmKeyInfo.
    Cause once the last item is trash-enabled, we keep this key from purging in OM DB.

    If the life time of key in deletedTable is less than recoverWindow, we didn't purge it in this time.
    So we use the difference between currentTime and modificationTime of key to check the situation.

  • Using trashTable to track trash.

    In part of deleting key,
    add cache-update of trashTable in OMKeyDeleteRequest and DB-operation of trashTable in OMKeyDeleteResponse

    In part of recovering trash,

    for OMTrashRecoverRequest
    remove OMKeyInfo in RepeatedOmKeyInfo from cache of trashTable and deletedTable.
    (not remove all RepeatedOmKeyInfo.)
    add OMKeyInfo to cache of keyTable.

    for OMTrashRecoverResponse
    remove OMKeyInfo in RepeatedOmKeyInfo of trashTable and deletedTable in OM DB.
    add OMKeyInfo to KeyTable in OM DB.

What is the link to the Apache JIRA

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

How was this patch tested?

Add UTs to track the OMTrashRecoverRequest and OMTrashRecoverResponse.

@cxorm
Copy link
Member Author

cxorm commented May 25, 2020

Rebased latest master-branch (#954 ) and trigger CI

@cxorm
Copy link
Member Author

cxorm commented May 25, 2020

I'm going to fixing violation.

@cxorm
Copy link
Member Author

cxorm commented May 26, 2020

This violation is not related to the change.
If anyone think it is improper, we could trigger actions again.
(I have test it with the same change here, and it passed all checks)

Copy link
Contributor

@bshashikant bshashikant left a comment

Choose a reason for hiding this comment

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

Thanks @cxorm for working on this. I have started the review and will have a deeper look into it.
@bharatviswa504 , can you plz review this as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

its better to name it as isTrashEnabled()?

Copy link
Contributor

Choose a reason for hiding this comment

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

rename as isTrashEnabled()?

Copy link
Contributor

Choose a reason for hiding this comment

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

can we rename the function shouldDelete() ? the name looks confusing

Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need to update the cache for both trash and deleted table here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, if a key gets deleted from a bucket which is trash enabled, i think we should just add it to the trash table, not the deleted table, otherwise it will lead to having duplicate entries in both trash and delete table. Once the trash interval expires, keys can be moved from trash table to deleted table using a background task which will then be cleaned by the KeyDeleting Service. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you @bshashikant for the review.
I'm working on this these days, and I would draw a picture for this question.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally, if a key gets deleted from a bucket which is trash enabled, i think we should just add it to the trash table, not the deleted table, otherwise it will lead to having duplicate entries in both trash and delete table. Once the trash interval expires, keys can be moved from trash table to deleted table using a background task which will then be cleaned by the KeyDeleting Service. What do you think?

Yeah, I agree that we shouldn't have duplicated entries in both and delete table.
This part would be updated .

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's hold off for now on this. There are some ideas being discussed around this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, thanks @bshashikant for the remind.

@cxorm
Copy link
Member Author

cxorm commented Jun 22, 2020

I am going to take it this week.

@cxorm
Copy link
Member Author

cxorm commented Jun 23, 2020

Rebased latest master-branch (#1109 ) and fixed conflict.

@bshashikant
Copy link
Contributor

/pending

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.

/pending

@github-actions
Copy link

Thank you very much for the patch. I am closing this PR temporarily as there was no activity recently and it is waiting for response from its author.

It doesn't mean that this PR is not important or ignored: feel free to reopen the PR at any time.

It only means that attention of committers is not required. We prefer to keep the review queue clean. This ensures PRs in need of review are more visible, which results in faster feedback for all PRs.

If you need ANY help to finish this PR, please contact the community on the mailing list or the slack channel."

@github-actions github-actions bot closed this Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants