You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "DaveTeng0 (via GitHub)" <gi...@apache.org> on 2023/09/06 00:30:12 UTC
[GitHub] [ozone] DaveTeng0 opened a new pull request, #5248: HDDS-9173 Invalidate snapshot cache once snapshot gets purged
DaveTeng0 opened a new pull request, #5248:
URL: https://github.com/apache/ozone/pull/5248
## What changes were proposed in this pull request?
Invalidate snapshot cache once snapshot gets purged
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-9173
## How was this patch tested?
manual testing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5248: HDDS-9173. Invalidate snapshot cache once snapshot gets purged
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5248:
URL: https://github.com/apache/ozone/pull/5248#discussion_r1317909166
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java:
##########
@@ -103,6 +103,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
trxnLogIndex, updatedSnapInfos, true);
updateSnapshotChainAndCache(omMetadataManager, fromSnapshot,
trxnLogIndex, updatedPathPreviousAndGlobalSnapshots);
+ ozoneManager.getOmSnapshotManager().getSnapshotCache()
Review Comment:
Can you please add test for it? Could be as simple as checking cache size after the purge finishes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] DaveTeng0 commented on pull request #5248: HDDS-9173. Invalidate snapshot cache once snapshot gets purged
Posted by "DaveTeng0 (via GitHub)" <gi...@apache.org>.
DaveTeng0 commented on PR #5248:
URL: https://github.com/apache/ozone/pull/5248#issuecomment-1709103941
> @DaveTeng0 Can you fix the checkstyle?
oh sure! Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5248: HDDS-9173. Invalidate snapshot cache once snapshot gets purged
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5248:
URL: https://github.com/apache/ozone/pull/5248#discussion_r1321971175
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java:
##########
@@ -103,6 +103,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
trxnLogIndex, updatedSnapInfos, true);
updateSnapshotChainAndCache(omMetadataManager, fromSnapshot,
trxnLogIndex, updatedPathPreviousAndGlobalSnapshots);
+ ozoneManager.getOmSnapshotManager().getSnapshotCache()
Review Comment:
Thanks @DaveTeng0
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] DaveTeng0 commented on a diff in pull request #5248: HDDS-9173. Invalidate snapshot cache once snapshot gets purged
Posted by "DaveTeng0 (via GitHub)" <gi...@apache.org>.
DaveTeng0 commented on code in PR #5248:
URL: https://github.com/apache/ozone/pull/5248#discussion_r1322053570
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestSnapshotDeletingService.java:
##########
@@ -188,6 +191,30 @@ public void testMultipleSnapshotKeyReclaim() throws Exception {
assertTableRowCount(deletedTable, 0);
verifySnapshotChain(delSnapInfo, null);
+
+ // verify the cache of purged snapshot
+ // /vol1/bucket2/bucket2snap1 has been cleaned up from cache map
+ SnapshotCache snapshotCache = om.getOmSnapshotManager().getSnapshotCache();
+ Method getDbMapMethod = snapshotCache.getClass()
Review Comment:
oh! you're absolutely right! let me change it!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] DaveTeng0 commented on a diff in pull request #5248: HDDS-9173. Invalidate snapshot cache once snapshot gets purged
Posted by "DaveTeng0 (via GitHub)" <gi...@apache.org>.
DaveTeng0 commented on code in PR #5248:
URL: https://github.com/apache/ozone/pull/5248#discussion_r1320198371
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java:
##########
@@ -103,6 +103,8 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
trxnLogIndex, updatedSnapInfos, true);
updateSnapshotChainAndCache(omMetadataManager, fromSnapshot,
trxnLogIndex, updatedPathPreviousAndGlobalSnapshots);
+ ozoneManager.getOmSnapshotManager().getSnapshotCache()
Review Comment:
Yup! sure definitely! @hemantk-12 please take a look the new verification part added in [TestSnapshotDeletingService.java](https://github.com/apache/ozone/pull/5248/commits/f7a4b3886cd67fa7ba1275aca9a70197bdab88ff#diff-4467e2d5f908fe0f61428c3328bd0a0c797d0b07e3a38ec40969f2264277ecba)
Feel free to let me know if any question, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #5248: HDDS-9173. Invalidate snapshot cache once snapshot gets purged
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #5248:
URL: https://github.com/apache/ozone/pull/5248#discussion_r1321975050
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestSnapshotDeletingService.java:
##########
@@ -188,6 +191,30 @@ public void testMultipleSnapshotKeyReclaim() throws Exception {
assertTableRowCount(deletedTable, 0);
verifySnapshotChain(delSnapInfo, null);
+
+ // verify the cache of purged snapshot
+ // /vol1/bucket2/bucket2snap1 has been cleaned up from cache map
+ SnapshotCache snapshotCache = om.getOmSnapshotManager().getSnapshotCache();
+ Method getDbMapMethod = snapshotCache.getClass()
Review Comment:
I think you can directly use [SnapshotCache#size()](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotCache.java#L94).
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestSnapshotDeletingService.java:
##########
@@ -188,6 +191,30 @@ public void testMultipleSnapshotKeyReclaim() throws Exception {
assertTableRowCount(deletedTable, 0);
verifySnapshotChain(delSnapInfo, null);
+
+ // verify the cache of purged snapshot
+ // /vol1/bucket2/bucket2snap1 has been cleaned up from cache map
+ SnapshotCache snapshotCache = om.getOmSnapshotManager().getSnapshotCache();
+ Method getDbMapMethod = snapshotCache.getClass()
+ .getDeclaredMethod("getDbMap");
+ getDbMapMethod.setAccessible(true);
+ ConcurrentHashMap<String,
+ ReferenceCounted<IOmMetadataReader, SnapshotCache>> dbMap =
+ (ConcurrentHashMap<String,
+ ReferenceCounted<IOmMetadataReader,
+ SnapshotCache>>) getDbMapMethod.invoke(snapshotCache);
+
+ assertEquals(2, dbMap.size());
+
+ Method getPendingEvictionListMethod =
Review Comment:
Similar to previous comment, may be create a public function for `pendingEvictionList#size`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] aswinshakil commented on pull request #5248: HDDS-9173. Invalidate snapshot cache once snapshot gets purged
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on PR #5248:
URL: https://github.com/apache/ozone/pull/5248#issuecomment-1709074870
@DaveTeng0 Can you fix the checkstyle?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hemantk-12 merged pull request #5248: HDDS-9173. Invalidate snapshot cache once snapshot gets purged
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 merged PR #5248:
URL: https://github.com/apache/ozone/pull/5248
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] DaveTeng0 commented on a diff in pull request #5248: HDDS-9173. Invalidate snapshot cache once snapshot gets purged
Posted by "DaveTeng0 (via GitHub)" <gi...@apache.org>.
DaveTeng0 commented on code in PR #5248:
URL: https://github.com/apache/ozone/pull/5248#discussion_r1322067841
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestSnapshotDeletingService.java:
##########
@@ -188,6 +191,30 @@ public void testMultipleSnapshotKeyReclaim() throws Exception {
assertTableRowCount(deletedTable, 0);
verifySnapshotChain(delSnapInfo, null);
+
+ // verify the cache of purged snapshot
+ // /vol1/bucket2/bucket2snap1 has been cleaned up from cache map
+ SnapshotCache snapshotCache = om.getOmSnapshotManager().getSnapshotCache();
+ Method getDbMapMethod = snapshotCache.getClass()
+ .getDeclaredMethod("getDbMap");
+ getDbMapMethod.setAccessible(true);
+ ConcurrentHashMap<String,
+ ReferenceCounted<IOmMetadataReader, SnapshotCache>> dbMap =
+ (ConcurrentHashMap<String,
+ ReferenceCounted<IOmMetadataReader,
+ SnapshotCache>>) getDbMapMethod.invoke(snapshotCache);
+
+ assertEquals(2, dbMap.size());
+
+ Method getPendingEvictionListMethod =
Review Comment:
yeah, that makes sense! updated! Thanks @hemantk-12 !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org