HDDS-13004. Snapshot Cache lock on a specific snapshotId #9210
Conversation
Change-Id: Ib988b6e862f4964bc5915f6e845c6905a8a650ee
|
@SaketaChalamchala can you review this patch? |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the SnapshotCache class to add support for snapshot-specific locking and improves the cleanup mechanism by extracting single-key cleanup logic into a separate method.
- Adds a new
lock(UUID snapshotId)method that acquires a write lock for a specific snapshot - Refactors the cleanup logic by extracting single-key cleanup into a new
cleanup(UUID evictionKey)method - Renames test method from
testGetHoldsWriteLocktotestLockHoldsWriteLockfor consistency - Adds test coverage for the new snapshot-specific lock method
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| SnapshotCache.java | Adds snapshot-specific lock method and refactors cleanup logic into two overloaded methods |
| TestSnapshotCache.java | Renames test method and adds test for new snapshot-specific lock functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Acquires a write lock on a specific snapshot database and returns an auto-closeble supplier for lock details. | ||
| * The lock ensures that the operations accessing the snapshot database are perfromed in a thread safe manner. The | ||
| * returned supplier automatically releases the lock acquired when closed, preveneting potential resource |
There was a problem hiding this comment.
Multiple spelling errors in the JavaDoc: 'auto-closeble' should be 'auto-closeable', 'perfromed' should be 'performed', 'preveneting' should be 'preventing'.
| * Acquires a write lock on a specific snapshot database and returns an auto-closeble supplier for lock details. | |
| * The lock ensures that the operations accessing the snapshot database are perfromed in a thread safe manner. The | |
| * returned supplier automatically releases the lock acquired when closed, preveneting potential resource | |
| * Acquires a write lock on a specific snapshot database and returns an auto-closeable supplier for lock details. | |
| * The lock ensures that the operations accessing the snapshot database are performed in a thread safe manner. The | |
| * returned supplier automatically releases the lock acquired when closed, preventing potential resource |
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java
Outdated
Show resolved
Hide resolved
Change-Id: I4496de9d403268a179b9ab65afb8575b07ad83a6
Change-Id: I9a6914f0b383486da7c95063a57ea64ba52dcec7
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -324,43 +336,49 @@ public OMLockDetails get() { | |||
| * If cache size exceeds soft limit, attempt to clean up and close the | |||
| instances that has zero reference count. | |||
There was a problem hiding this comment.
Grammatical error: 'instances that has' should be 'instances that have' for subject-verb agreement.
| instances that has zero reference count. | |
| instances that have zero reference count. |
smengcl
left a comment
smengcl
left a comment
There was a problem hiding this comment.
Thanks @swamirishi . lgtm
| AtomicReference<OMLockDetails> lockDetails = new AtomicReference<>(lockFunction.get()); | ||
| if (lockDetails.get().isLockAcquired()) { | ||
| cleanup(true); | ||
| cleanupFunction.get(); |
There was a problem hiding this comment.
why are we closing the snapshot instance upon acquiring a lock?
There was a problem hiding this comment.
We don't want a rocksdb open in the cache once a write lock has been acquired. We would be using this for atomically switching the rocksdb directory after defragging a snapshot
|
thank you @smengcl and @sadanand48 for reviewing the patch |
|
@swamirishi please set fix version when resolving Jira issue after PR merge |
(cherry picked from commit 2806bae)
(cherry picked from commit 2806bae)
…pache#9210) Change-Id: I3e578f73f909722b90c7ed8314ebc617c4d9c351
4 participants
What changes were proposed in this pull request?
A Snapshot Cache lock needs to be implemented which should prevent the rocksdb to be opened while the lock is held.
The idea is, the snapshot can't be opened for other purposes during snapshot defragmentation (note: snapshot compaction is not DB compaction). Essentially, an exclusive lock should be taken on the snapshot during the process.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-13004
How was this patch tested?
Added unit tests