From ae562e5c3dcad758b23fdd696831e53ee73d4c66 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Mon, 25 Mar 2024 22:16:12 -0700 Subject: [PATCH 1/2] Snapshot chain fix --- .../hadoop/ozone/om/helpers/SnapshotInfo.java | 2 +- .../hadoop/ozone/om/SnapshotChainManager.java | 9 +++-- .../snapshot/OMSnapshotPurgeRequest.java | 4 +++ .../snapshot/OMSnapshotPurgeResponse.java | 8 +++-- ...TestOMSnapshotPurgeRequestAndResponse.java | 33 +++++++++++++------ 5 files changed, 37 insertions(+), 19 deletions(-) diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/SnapshotInfo.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/SnapshotInfo.java index fd84ffe06605..5a1ecee26f9d 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/SnapshotInfo.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/SnapshotInfo.java @@ -718,7 +718,7 @@ public SnapshotInfo copyObject() { @Override public String toString() { return "SnapshotInfo{" + - ", snapshotId: '" + snapshotId + '\'' + + "snapshotId: '" + snapshotId + '\'' + ", name: '" + name + "'," + ", volumeName: '" + volumeName + '\'' + ", bucketName: '" + bucketName + '\'' + diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotChainManager.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotChainManager.java index 60353590e75c..99ca87c32d73 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotChainManager.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotChainManager.java @@ -362,15 +362,14 @@ public synchronized void updateSnapshot(SnapshotInfo snapshotInfo) { public synchronized boolean deleteSnapshot(SnapshotInfo snapshotInfo) throws IOException { validateSnapshotChain(); - boolean status = deleteSnapshotGlobal(snapshotInfo.getSnapshotId()) && + return deleteSnapshotGlobal(snapshotInfo.getSnapshotId()) && deleteSnapshotPath(snapshotInfo.getSnapshotPath(), snapshotInfo.getSnapshotId()); - if (status) { - snapshotIdToTableKey.remove(snapshotInfo.getSnapshotId()); - } - return status; } + public synchronized void deleteSnapshotFromSnapshotIdToTableKey(UUID snapshotId) { + snapshotIdToTableKey.remove(snapshotId); + } /** * Get latest global snapshot in snapshot chain. */ diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java index 3f4d746adb54..02c44e0084d8 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java @@ -102,7 +102,11 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, TermIn trxnLogIndex, updatedSnapInfos); updateSnapshotChainAndCache(omMetadataManager, fromSnapshot, trxnLogIndex, updatedPathPreviousAndGlobalSnapshots); + // Remove and close snapshot's RocksDB instance from SnapshotCache. ozoneManager.getOmSnapshotManager().invalidateCacheEntry(fromSnapshot.getSnapshotId()); + // Update SnapshotInfoTable cache. + ozoneManager.getMetadataManager().getSnapshotInfoTable() + .addCacheEntry(new CacheKey<>(fromSnapshot.getTableKey()), CacheValue.get(trxnLogIndex)); } omClientResponse = new OMSnapshotPurgeResponse(omResponse.build(), diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java index b8db58d7fd9e..43cfc92e24d0 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java @@ -84,8 +84,9 @@ protected void addToDBBatch(OMMetadataManager omMetadataManager, updateSnapInfo(metadataManager, batchOperation, updatedPreviousAndGlobalSnapInfos); for (String dbKey: snapshotDbKeys) { + // Skip the cache here because snapshot is purged from cache in OMSnapshotPurgeRequest. SnapshotInfo snapshotInfo = omMetadataManager - .getSnapshotInfoTable().get(dbKey); + .getSnapshotInfoTable().getSkipCache(dbKey); // Even though snapshot existed when SnapshotDeletingService // was running. It might be deleted in the previous run and // the DB might not have been updated yet. So snapshotInfo @@ -96,8 +97,9 @@ protected void addToDBBatch(OMMetadataManager omMetadataManager, // Delete Snapshot checkpoint directory. deleteCheckpointDirectory(omMetadataManager, snapshotInfo); - omMetadataManager.getSnapshotInfoTable().deleteWithBatch(batchOperation, - dbKey); + omMetadataManager.getSnapshotInfoTable().deleteWithBatch(batchOperation, dbKey); + ((OmMetadataManagerImpl) omMetadataManager).getSnapshotChainManager() + .deleteSnapshotFromSnapshotIdToTableKey(snapshotInfo.getSnapshotId()); } } diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotPurgeRequestAndResponse.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotPurgeRequestAndResponse.java index 71882c3423e0..7c0a5c4ff875 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotPurgeRequestAndResponse.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotPurgeRequestAndResponse.java @@ -67,6 +67,7 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.any; import static org.mockito.Mockito.anyString; @@ -77,8 +78,6 @@ * Tests OMSnapshotPurgeRequest class. */ public class TestOMSnapshotPurgeRequestAndResponse { - - private BatchOperation batchOperation; private List checkpointPaths = new ArrayList<>(); private OzoneManager ozoneManager; @@ -177,7 +176,6 @@ private void createSnapshotCheckpoint(String snapshotName) throws Exception { private void createSnapshotCheckpoint(String volume, String bucket, String snapshotName) throws Exception { - batchOperation = omMetadataManager.getStore().initBatchOperation(); OMRequest omRequest = OMRequestTestUtils .createSnapshotRequest(volume, bucket, snapshotName); // Pre-Execute OMSnapshotCreateRequest. @@ -188,9 +186,10 @@ private void createSnapshotCheckpoint(String volume, OMSnapshotCreateResponse omClientResponse = (OMSnapshotCreateResponse) omSnapshotCreateRequest.validateAndUpdateCache(ozoneManager, 1); // Add to batch and commit to DB. - omClientResponse.addToDBBatch(omMetadataManager, batchOperation); - omMetadataManager.getStore().commitBatchOperation(batchOperation); - batchOperation.close(); + try (BatchOperation batchOperation = omMetadataManager.getStore().initBatchOperation()) { + omClientResponse.addToDBBatch(omMetadataManager, batchOperation); + omMetadataManager.getStore().commitBatchOperation(batchOperation); + } String key = SnapshotInfo.getTableKey(volume, bucket, snapshotName); SnapshotInfo snapshotInfo = @@ -226,9 +225,10 @@ private void purgeSnapshots(OMRequest snapshotPurgeRequest) omSnapshotPurgeRequest.validateAndUpdateCache(ozoneManager, 200L); // Commit to DB. - batchOperation = omMetadataManager.getStore().initBatchOperation(); - omSnapshotPurgeResponse.checkAndUpdateDB(omMetadataManager, batchOperation); - omMetadataManager.getStore().commitBatchOperation(batchOperation); + try (BatchOperation batchOperation = omMetadataManager.getStore().initBatchOperation()) { + omSnapshotPurgeResponse.checkAndUpdateDB(omMetadataManager, batchOperation); + omMetadataManager.getStore().commitBatchOperation(batchOperation); + } } @Test @@ -238,7 +238,20 @@ public void testValidateAndUpdateCache() throws Exception { assertFalse(omMetadataManager.getSnapshotInfoTable().isEmpty()); OMRequest snapshotPurgeRequest = createPurgeKeysRequest( snapshotDbKeysToPurge); - purgeSnapshots(snapshotPurgeRequest); + + OMSnapshotPurgeRequest omSnapshotPurgeRequest = preExecute(snapshotPurgeRequest); + + OMSnapshotPurgeResponse omSnapshotPurgeResponse = (OMSnapshotPurgeResponse) + omSnapshotPurgeRequest.validateAndUpdateCache(ozoneManager, 200L); + + for (String snapshotTableKey: snapshotDbKeysToPurge) { + assertNull(omMetadataManager.getSnapshotInfoTable().get(snapshotTableKey)); + } + + try (BatchOperation batchOperation = omMetadataManager.getStore().initBatchOperation()) { + omSnapshotPurgeResponse.checkAndUpdateDB(omMetadataManager, batchOperation); + omMetadataManager.getStore().commitBatchOperation(batchOperation); + } // Check if the entries are deleted. assertTrue(omMetadataManager.getSnapshotInfoTable().isEmpty()); From 0703d765934c5d4bb90c95da2eb41377a38edfb3 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Tue, 26 Mar 2024 14:44:08 -0700 Subject: [PATCH 2/2] addressed review comments --- .../org/apache/hadoop/ozone/om/SnapshotChainManager.java | 9 +++++---- .../om/response/snapshot/OMSnapshotPurgeResponse.java | 2 -- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotChainManager.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotChainManager.java index 99ca87c32d73..60353590e75c 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotChainManager.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotChainManager.java @@ -362,14 +362,15 @@ public synchronized void updateSnapshot(SnapshotInfo snapshotInfo) { public synchronized boolean deleteSnapshot(SnapshotInfo snapshotInfo) throws IOException { validateSnapshotChain(); - return deleteSnapshotGlobal(snapshotInfo.getSnapshotId()) && + boolean status = deleteSnapshotGlobal(snapshotInfo.getSnapshotId()) && deleteSnapshotPath(snapshotInfo.getSnapshotPath(), snapshotInfo.getSnapshotId()); + if (status) { + snapshotIdToTableKey.remove(snapshotInfo.getSnapshotId()); + } + return status; } - public synchronized void deleteSnapshotFromSnapshotIdToTableKey(UUID snapshotId) { - snapshotIdToTableKey.remove(snapshotId); - } /** * Get latest global snapshot in snapshot chain. */ diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java index 43cfc92e24d0..16411baa96e8 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java @@ -98,8 +98,6 @@ protected void addToDBBatch(OMMetadataManager omMetadataManager, // Delete Snapshot checkpoint directory. deleteCheckpointDirectory(omMetadataManager, snapshotInfo); omMetadataManager.getSnapshotInfoTable().deleteWithBatch(batchOperation, dbKey); - ((OmMetadataManagerImpl) omMetadataManager).getSnapshotChainManager() - .deleteSnapshotFromSnapshotIdToTableKey(snapshotInfo.getSnapshotId()); } }