HDDS-10250. Use SnapshotId as key in SnapshotCache #6139
Conversation
hemantk-12
changed the title
aswinshakil
left a comment
aswinshakil
left a comment
There was a problem hiding this comment.
Thanks for making this change @hemantk-12. The changes mostly look good to me. One more pair of eyes on this should be good. I'll work on HDDS-10265 once this gets merged to complete the whole change.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java
Outdated
Show resolved
Hide resolved
smengcl
left a comment
smengcl
left a comment
There was a problem hiding this comment.
Thanks @hemantk-12 . Change looks straight-forward. No compatibility concern as far as can I see since this is OM internal in-memory state. lgtm
hemantk-12
force-pushed
the
HDDS-10250
branch
3 times, most recently
from
1e1580c to
59220cb
Compare
hemantk-12
force-pushed
the
HDDS-10250
branch
from
59220cb to
36531cb
Compare
|
|
||
| // Block snapshot from loading when it is no longer active e.g. DELETED, | ||
| // unless this is called from SnapshotDeletingService. | ||
| checkSnapshotActive(snapshotInfo, true); |
There was a problem hiding this comment.
@aswinshakil this is not needed. Please correct me if I'm wrong.
There was a problem hiding this comment.
We needed it as a rail guard so that DELETED snapshot is not loaded by other services. But I don't think we would need it. Reason is that we already block FS calls to snapshot, We check snapshot status in snapDiff for each page. Make sense to remove this.
aswinshakil
left a comment
aswinshakil
left a comment
There was a problem hiding this comment.
LGTM+1. Thanks for the patch @hemantk-12
|
Thanks @aswinshakil and @smengcl for the review. |
(cherry picked from commit c35e99f)
(cherry picked from commit c35e99f)
(cherry picked from commit c35e99f)
(cherry picked from commit c35e99f)
(cherry picked from commit c35e99f)
…e#6139) Change-Id: I295af74d4f6743ed04635e043a0b5782333dec69
3 participants
What changes were proposed in this pull request?
Currently,
SnapshotName(/volumeName/bucketName/snapshotName) is used as key in SnapshotCache which makes it hard for operations like Snapshot rename. Discussions: #6006 (review)This change is to use
SnapshotIdas key in SnapshotCache rather than SnapshotName becauseSnapshotIdis unique and is never changed throughout the snapshot's lifecycle.What is the link to the Apache JIRA
HDDS-10250
How was this patch tested?
Updated existing unit tests to work with SnapshotId. No new tests were added.