Skip to content

HDDS-8686. [Snapshot] SnapshotDeletingService to reclaim old versions of key.#4811

Merged
aswinshakil merged 7 commits intoapache:masterfrom
aswinshakil:HDDS-8686
Jun 17, 2023
Merged

HDDS-8686. [Snapshot] SnapshotDeletingService to reclaim old versions of key.#4811
aswinshakil merged 7 commits intoapache:masterfrom
aswinshakil:HDDS-8686

Conversation

@aswinshakil
Copy link
Copy Markdown
Member

@aswinshakil aswinshakil commented May 31, 2023

What changes were proposed in this pull request?

When a key is overwritten, it will still have the same object ID as the previous version of the key. So only the object ID check between snapshots is not enough to prove the key is same. This patch does an additional check on the OMBlockLocationInfoGroup.

What is the link to the Apache JIRA

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

How was this patch tested?

The patch was tested using updated existing test.
Screen Shot 2023-06-01 at 11 30 35 AM

@aswinshakil aswinshakil added the snapshot https://issues.apache.org/jira/browse/HDDS-6517 label May 31, 2023
@aswinshakil aswinshakil self-assigned this May 31, 2023
@sumitagrawl
Copy link
Copy Markdown
Contributor

sumitagrawl commented Jun 7, 2023

@aswinshakil I am not getting when object Id is same but content is different ? I can see only case is HSync.
So for Hsync, we can just check metadata if it belongs to Hsync or not, if Belongs, then there is a possibility of object can be different.
For other cases, it does not need to compare details of object.

Further, in delete flow, objectId will be unique, so it does not need compare any details, all delete task is independent, only objectId compare is enough.

@sumitagrawl sumitagrawl self-requested a review June 7, 2023 05:50
@aswinshakil
Copy link
Copy Markdown
Member Author

@sumitagrawl When we overwrite a key, the object ID remains the same but the key is different.

Although at a time only one version exists (by default bucket version is turned off). During snapshot deletion, multiple versions of the keys with the same objectID are moved to either next snapshot or active db's deletedTable.

In this case, we might need to check the block locations as well to confirm if it is the same version of the key.

@sumitagrawl
Copy link
Copy Markdown
Contributor

@aswinshakil Thanks for the info, overwrite case can have same objectId but different version of content.
IMO,

  • Need to have unique key for file key in deletedTable (similar to uncommited block handling in commit request), this will ensure no block leak due to overwrite of key when conflict.
  • keyDeletingService need compare objectId (as existing) and one of blockId if matching
  • SnapshotDeletingService also need have similar comparison with additional blockID if matching
  • Snapdiff also need have similar comparison for identify if object is same or different

I think, we can have a change in KeyInfo have have additional field - version, which can be auto-incremented in case of overwrite to distinguish different object and also for debugging identifying overwrite. This can be used to replace blockId compare.

Copy link
Copy Markdown
Contributor

@swamirishi swamirishi left a comment

Choose a reason for hiding this comment

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

Minor comments otherwise changes look good

return Pair.of(keyBlocksList, keysToModify);
}

private boolean versionExistsInPreviousSnapshot(OmKeyInfo omKeyInfo,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Parameter names nomenclature could be better.

Copy link
Copy Markdown
Contributor

@sumitagrawl sumitagrawl left a comment

Choose a reason for hiding this comment

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

@aswinshakil Thanks working over this, have few comments and query ...

smengcl

This comment was marked as duplicate.

Copy link
Copy Markdown
Contributor

@smengcl smengcl left a comment

Choose a reason for hiding this comment

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

+1, pending Sumit's comments resolution

@aswinshakil
Copy link
Copy Markdown
Member Author

Thanks @smengcl @swamirishi @sumitagrawl for the review.

@aswinshakil aswinshakil merged commit bdd26ee into apache:master Jun 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

snapshot https://issues.apache.org/jira/browse/HDDS-6517

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants