From 8cc4c77a5973566a294e5818e64a272b6181757a Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Fri, 5 May 2023 19:36:48 -0700 Subject: [PATCH 1/3] Added creationTime in CreateSnapshotRequest to fix SnapshotChain restore failure --- .../hadoop/ozone/om/helpers/SnapshotInfo.java | 10 +- .../ozone/om/TestOzoneManagerHASnapshot.java | 120 +++++++ .../src/main/proto/OmClientProtocol.proto | 1 + .../hadoop/ozone/om/OmSnapshotManager.java | 6 +- .../hadoop/ozone/om/SnapshotChainManager.java | 318 ++++++++---------- .../snapshot/OMSnapshotCreateRequest.java | 5 +- .../snapshot/OMSnapshotDeleteRequest.java | 2 +- .../ozone/om/TestOmSnapshotManager.java | 4 +- .../ozone/om/request/OMRequestTestUtils.java | 4 +- .../snapshot/TestOMSnapshotDeleteRequest.java | 3 +- .../TestOMSnapshotCreateResponse.java | 4 +- .../TestOMSnapshotDeleteResponse.java | 4 +- 12 files changed, 291 insertions(+), 190 deletions(-) create mode 100644 hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHASnapshot.java 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 bc297a92fd49..31f67fbdba9e 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 @@ -27,8 +27,6 @@ import com.google.common.base.Preconditions; -import org.apache.hadoop.util.Time; - import java.time.format.DateTimeFormatter; import java.time.Instant; import java.time.ZonedDateTime; @@ -479,15 +477,15 @@ public static String generateName(long initialTime) { public static SnapshotInfo newInstance(String volumeName, String bucketName, String snapshotName, - String snapshotId) { + String snapshotId, + long creationTime) { SnapshotInfo.Builder builder = new SnapshotInfo.Builder(); - long initialTime = Time.now(); if (StringUtils.isBlank(snapshotName)) { - snapshotName = generateName(initialTime); + snapshotName = generateName(creationTime); } builder.setSnapshotID(snapshotId) .setName(snapshotName) - .setCreationTime(initialTime) + .setCreationTime(creationTime) .setDeletionTime(INVALID_TIMESTAMP) .setPathPreviousSnapshotID(INITIAL_SNAPSHOT_ID) .setGlobalPreviousSnapshotID(INITIAL_SNAPSHOT_ID) diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHASnapshot.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHASnapshot.java new file mode 100644 index 000000000000..a44fd20dac3a --- /dev/null +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHASnapshot.java @@ -0,0 +1,120 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.ozone.om; + +import org.apache.commons.lang3.RandomStringUtils; +import org.apache.hadoop.hdds.utils.db.Table; +import org.apache.hadoop.hdds.utils.db.TableIterator; +import org.apache.hadoop.ozone.client.OzoneBucket; +import org.apache.hadoop.ozone.om.helpers.SnapshotInfo; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; + +import java.io.IOException; +import java.time.Duration; +import java.util.ArrayList; +import java.util.List; + +import static org.apache.hadoop.ozone.OzoneConsts.OM_KEY_PREFIX; +import static org.awaitility.Awaitility.await; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Tests snapshot in OM HA setup. + */ +@Timeout(300) +public class TestOzoneManagerHASnapshot extends TestOzoneManagerHA { + + /** + * Test snapshotNames are unique among OM nodes when snapshotName is not + * passed or empty. + */ + @Test + public void testUniqueSnapshotName() throws Exception { + OzoneBucket ozoneBucket = setupBucket(); + String volumeName = ozoneBucket.getVolumeName(); + String bucketName = ozoneBucket.getName(); + + createKey(ozoneBucket); + + getObjectStore().createSnapshot(volumeName, bucketName, ""); + List ozoneManagers = getCluster().getOzoneManagersList(); + List snapshotNames = new ArrayList<>(); + + for (OzoneManager ozoneManager : ozoneManagers) { + await().atMost(Duration.ofSeconds(120)) + .until(() -> { + String snapshotPrefix = OM_KEY_PREFIX + volumeName + + OM_KEY_PREFIX + bucketName; + SnapshotInfo snapshotInfo = null; + try (TableIterator> + iterator = ozoneManager.getMetadataManager() + .getSnapshotInfoTable().iterator(snapshotPrefix)) { + while (iterator.hasNext()) { + snapshotInfo = iterator.next().getValue(); + } + } catch (IOException e) { + throw new RuntimeException(e); + } + + if (snapshotInfo != null) { + snapshotNames.add(snapshotInfo.getName()); + } + return snapshotInfo != null; + }); + } + + assertEquals(1, snapshotNames.stream().distinct().count()); + assertNotNull(snapshotNames.get(0)); + assertTrue(snapshotNames.get(0).startsWith("s")); + } + + @Test + public void testSnapshotChainManagerRestore() throws Exception { + List ozoneBuckets = new ArrayList<>(); + List volumeNames = new ArrayList<>(); + List bucketNames = new ArrayList<>(); + + for (int i = 0; i < 10; i++) { + OzoneBucket ozoneBucket = setupBucket(); + ozoneBuckets.add(ozoneBucket); + volumeNames.add(ozoneBucket.getVolumeName()); + bucketNames.add(ozoneBucket.getName()); + } + + for (int i = 0; i < 100; i++) { + int index = i % 10; + createKey(ozoneBuckets.get(index)); + String snapshot1 = "snapshot-" + RandomStringUtils.randomNumeric(5); + getObjectStore().createSnapshot(volumeNames.get(index), + bucketNames.get(index), snapshot1); + } + + // Restart leader OM + OzoneManager omLeader = getCluster().getOMLeader(); + getCluster().shutdownOzoneManager(omLeader); + getCluster().restartOzoneManager(omLeader, true); + + await().atMost(Duration.ofSeconds(180)) + .until(() -> getCluster().getOMLeader() != null); + } +} diff --git a/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto b/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto index a0d5fcbf6cc5..390d5e518e97 100644 --- a/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto +++ b/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto @@ -1686,6 +1686,7 @@ message CreateSnapshotRequest { optional string bucketName = 2; optional string snapshotName = 3; optional string snapshotId = 4; + optional uint64 creationTime = 5; } message ListSnapshotRequest { diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java index 0120ca892fa2..9a94e013b11c 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java @@ -386,8 +386,10 @@ public static DBCheckpoint createOmSnapshotCheckpoint( .writeLock().unlock(); } - LOG.info("Created checkpoint : {} for snapshot {}", - dbCheckpoint.getCheckpointLocation(), snapshotInfo.getName()); + if (dbCheckpoint != null) { + LOG.info("Created checkpoint : {} for snapshot {}", + dbCheckpoint.getCheckpointLocation(), snapshotInfo.getName()); + } final RocksDBCheckpointDiffer dbCpDiffer = store.getRocksDBCheckpointDiffer(); 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 8dc6c703e0bb..ef78434f8b99 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 @@ -33,26 +33,27 @@ /** * This class is used for creating and accessing Snapshot Chains. - * + *

* The snapshot chain maintains the in-memory sequence of snapshots * created in chronological order. There are two such snapshots maintained * i.) Path based snapshot chain, sequence of snapshots created for a * given /volume/bucket * ii.) Global snapshot chain, sequence of all snapshots created in order - * + *

* On start, the snapshot chains are initialized from the on disk * SnapshotInfoTable from the om RocksDB. */ public class SnapshotChainManager { - private LinkedHashMap snapshotChainGlobal; - private Map> - snapshotChainPath; - private Map latestPathSnapshotID; - private String latestGlobalSnapshotID; - private Map snapshotIdToTableKey; private static final Logger LOG = LoggerFactory.getLogger(SnapshotChainManager.class); + private final LinkedHashMap snapshotChainGlobal; + private final Map> + snapshotChainPath; + private String latestGlobalSnapshotID; + private final Map latestPathSnapshotID; + private final Map snapshotIdToTableKey; + public SnapshotChainManager(OMMetadataManager metadataManager) throws IOException { snapshotChainGlobal = new LinkedHashMap<>(); @@ -83,10 +84,10 @@ private void addSnapshotGlobal(String snapshotID, } if (prevGlobalID != null && !snapshotChainGlobal.containsKey(prevGlobalID)) { - throw new IOException("Snapshot Chain corruption: " - + "previous snapshotID given but no associated snapshot " - + "found in snapshot chain: SnapshotID " - + prevGlobalID); + throw new IOException(String.format("Snapshot chain corruption. " + + "Previous snapshotId: %s is set for snapshotId: %s but no " + + "associated snapshot found in snapshot chain.", prevGlobalID, + snapshotID)); } snapshotChainGlobal.put(snapshotID, new SnapshotChainInfo(snapshotID, prevGlobalID, null)); @@ -115,10 +116,10 @@ private void addSnapshotPath(String snapshotPath, (!snapshotChainPath .get(snapshotPath) .containsKey(prevPathID)))) { - throw new IOException("Snapshot Chain corruption: " - + "previous snapshotID given but no associated snapshot " - + "found in snapshot chain: SnapshotID " - + prevPathID); + throw new IOException(String.format("Snapshot chain corruption. " + + "Previous snapshotId: %s is set for snapshotId: %s but no " + + "associated snapshot found in snapshot chain.", prevPathID, + snapshotID)); } if (prevPathID != null && @@ -151,14 +152,16 @@ private boolean deleteSnapshotGlobal(String snapshotID) throws IOException { String prev = snapshotChainGlobal.get(snapshotID).getPreviousSnapshotID(); if (prev != null && !snapshotChainGlobal.containsKey(prev)) { - throw new IOException("Snapshot chain corruption: snapshot node to be " - + "deleted has prev node element not found in snapshot chain: " - + "SnapshotID " + prev); + throw new IOException(String.format("Snapshot chain corruption. " + + "SnapshotId: %s to be deleted has previous snapshotId: %s " + + "but associated snapshot is not found in snapshot chain.", + snapshotID, prev)); } if (next != null && !snapshotChainGlobal.containsKey(next)) { - throw new IOException("Snapshot chain corruption: snapshot node to be " - + "deleted has next node element not found in snapshot chain: " - + "SnapshotID " + next); + throw new IOException(String.format("Snapshot chain corruption. " + + "SnapshotId: {%s} to be deleted has next snapshotId: %s " + + "but associated snapshot is not found in snapshot chain.", + snapshotID, next)); } snapshotChainGlobal.remove(snapshotID); if (next != null) { @@ -173,8 +176,8 @@ private boolean deleteSnapshotGlobal(String snapshotID) throws IOException { } } else { // snapshotID not found in snapshot chain, log warning and return - LOG.warn("Snapshot chain: snapshotID not found: SnapshotID {}", - snapshotID); + LOG.warn("Snapshot chain corruption. SnapshotID: {} is not found in " + + "snapshot chain.", snapshotID); status = false; } @@ -183,53 +186,47 @@ private boolean deleteSnapshotGlobal(String snapshotID) throws IOException { private boolean deleteSnapshotPath(String snapshotPath, String snapshotID) throws IOException { - boolean status = true; if (snapshotChainPath.containsKey(snapshotPath) && - snapshotChainPath - .get(snapshotPath) - .containsKey(snapshotID)) { + snapshotChainPath.get(snapshotPath).containsKey(snapshotID)) { // reset prev and next snapshot entries in chain ordered list // for node removal - String next = snapshotChainPath + String nextSnapshotID = snapshotChainPath .get(snapshotPath) .get(snapshotID) .getNextSnapshotID(); - String prev = snapshotChainPath + String previousSnapshotID = snapshotChainPath .get(snapshotPath) .get(snapshotID) .getPreviousSnapshotID(); - if (prev != null && - !snapshotChainPath - .get(snapshotPath) - .containsKey(prev)) { - throw new IOException("Snapshot chain corruption: snapshot node to " - + "be deleted has prev node element not found in snapshot " - + "chain: Snapshot path " + snapshotPath + ", SnapshotID " - + prev); + if (previousSnapshotID != null && + !snapshotChainPath.get(snapshotPath) + .containsKey(previousSnapshotID)) { + throw new IOException(String.format("Snapshot chain corruption. " + + "SnapshotId: %s at snapshotPath: %s to be deleted has " + + "previous snapshotId: %s but associated snapshot is not " + + "found in snapshot chain.", snapshotID, snapshotPath, + previousSnapshotID)); } - if (next != null && !snapshotChainPath - .get(snapshotPath) - .containsKey(next)) { - throw new IOException("Snapshot chain corruption: snapshot node to " - + "be deleted has next node element not found in snapshot " - + "chain: Snapshot path " + snapshotPath + ", SnapshotID " - + next); + if (nextSnapshotID != null && !snapshotChainPath.get(snapshotPath) + .containsKey(nextSnapshotID)) { + throw new IOException(String.format("Snapshot chain corruption. " + + "SnapshotId: %s at snapshotPath: %s to be deleted has next " + + "snapshotId: %s but associated snapshot is not found in " + + "snapshot chain.", snapshotID, snapshotPath, + nextSnapshotID)); } - snapshotChainPath - .get(snapshotPath) - .remove(snapshotID); - if (next != null) { - snapshotChainPath - .get(snapshotPath) - .get(next) - .setPreviousSnapshotID(prev); + + snapshotChainPath.get(snapshotPath).remove(snapshotID); + if (nextSnapshotID != null) { + snapshotChainPath.get(snapshotPath) + .get(nextSnapshotID) + .setPreviousSnapshotID(previousSnapshotID); } - if (prev != null) { - snapshotChainPath - .get(snapshotPath) - .get(prev) - .setNextSnapshotID(next); + if (previousSnapshotID != null) { + snapshotChainPath.get(snapshotPath) + .get(previousSnapshotID) + .setNextSnapshotID(nextSnapshotID); } // remove path if no entries if (snapshotChainPath.get(snapshotPath).isEmpty()) { @@ -238,20 +235,17 @@ private boolean deleteSnapshotPath(String snapshotPath, // remove from latest list if necessary if (latestPathSnapshotID.get(snapshotPath).equals(snapshotID)) { latestPathSnapshotID.remove(snapshotPath); - if (prev != null) { - latestPathSnapshotID.put(snapshotPath, prev); + if (previousSnapshotID != null) { + latestPathSnapshotID.put(snapshotPath, previousSnapshotID); } } - + return true; } else { // snapshotID not found in snapshot chain, log warning and return - LOG.warn("Snapshot chain: snapshotID not found: Snapshot path {}," + - " SnapshotID {}", - snapshotPath, snapshotID); - status = false; + LOG.warn("Snapshot chain corruption. SnapshotId: {} is not in chain " + + "found for snapshot path {}.", snapshotID, snapshotPath); + return false; } - - return status; } /** @@ -259,7 +253,7 @@ private boolean deleteSnapshotPath(String snapshotPath, * @param metadataManager OMMetadataManager */ private void loadFromSnapshotInfoTable(OMMetadataManager metadataManager) - throws IOException { + throws IOException { // read from snapshotInfo table to populate // snapshot chains - both global and local path try (TableIterator> @@ -275,38 +269,37 @@ private void loadFromSnapshotInfoTable(OMMetadataManager metadataManager) kv = keyIter.next(); snaps.put(kv.getValue().getCreationTime(), kv.getValue()); } - for (SnapshotInfo sinfo : snaps.values()) { - addSnapshot(sinfo); + for (SnapshotInfo snapshotInfo : snaps.values()) { + addSnapshot(snapshotInfo); } } } /** * Add snapshot to snapshot chain. - * @param sinfo SnapshotInfo of snapshot to add to chain. + * @param snapshotInfo SnapshotInfo of snapshot to add to chain. */ - public void addSnapshot(SnapshotInfo sinfo) throws IOException { - addSnapshotGlobal(sinfo.getSnapshotID(), - sinfo.getGlobalPreviousSnapshotID()); - addSnapshotPath(sinfo.getSnapshotPath(), - sinfo.getSnapshotID(), - sinfo.getPathPreviousSnapshotID()); + public void addSnapshot(SnapshotInfo snapshotInfo) throws IOException { + addSnapshotGlobal(snapshotInfo.getSnapshotID(), + snapshotInfo.getGlobalPreviousSnapshotID()); + addSnapshotPath(snapshotInfo.getSnapshotPath(), + snapshotInfo.getSnapshotID(), snapshotInfo.getPathPreviousSnapshotID()); // store snapshot ID to snapshot DB table key in the map - snapshotIdToTableKey.put(sinfo.getSnapshotID(), sinfo.getTableKey()); + snapshotIdToTableKey.put(snapshotInfo.getSnapshotID(), + snapshotInfo.getTableKey()); } /** * Delete snapshot from snapshot chain. - * @param sinfo SnapshotInfo of snapshot to remove from chain. + * @param snapshotInfo SnapshotInfo of snapshot to remove from chain. * @return boolean */ - public boolean deleteSnapshot(SnapshotInfo sinfo) throws IOException { - boolean status; - - status = deleteSnapshotGlobal(sinfo.getSnapshotID()) && - deleteSnapshotPath(sinfo.getSnapshotPath(), sinfo.getSnapshotID()); + public boolean deleteSnapshot(SnapshotInfo snapshotInfo) throws IOException { + boolean status = deleteSnapshotGlobal(snapshotInfo.getSnapshotID()) && + deleteSnapshotPath(snapshotInfo.getSnapshotPath(), + snapshotInfo.getSnapshotID()); if (status) { - snapshotIdToTableKey.remove(sinfo.getSnapshotID()); + snapshotIdToTableKey.remove(snapshotInfo.getSnapshotID()); } return status; } @@ -336,19 +329,13 @@ public String getLatestPathSnapshot(String snapshotPath) { * @param snapshotID String, snapshot UUID * @return boolean */ - public boolean hasNextGlobalSnapshot(String snapshotID) - throws NoSuchElementException { - boolean hasNext = false; + public boolean hasNextGlobalSnapshot(String snapshotID) { if (!snapshotChainGlobal.containsKey(snapshotID)) { - LOG.error("no snapshot for provided snapshotID {}", snapshotID); - throw new NoSuchElementException("No snapshot: " + snapshotID); - } - if (snapshotChainGlobal - .get(snapshotID) - .getNextSnapshotID() != null) { - hasNext = true; + LOG.error("No snapshot for provided snapshotId: {}", snapshotID); + throw new NoSuchElementException(String.format("SnapshotId: %s is not " + + "found in snapshot chain.", snapshotID)); } - return hasNext; + return snapshotChainGlobal.get(snapshotID).getNextSnapshotID() != null; } /** @@ -357,16 +344,14 @@ public boolean hasNextGlobalSnapshot(String snapshotID) * @return String, snapshot UUID of next snapshot in chain from * snapshotID */ - public String nextGlobalSnapshot(String snapshotID) - throws NoSuchElementException { + public String nextGlobalSnapshot(String snapshotID) { if (!hasNextGlobalSnapshot(snapshotID)) { - LOG.error("no following snapshot for provided snapshotID {}", snapshotID); - throw new NoSuchElementException("no following snapshot from: " - + snapshotID); + LOG.error("No following snapshot found in snapshot chain for provided " + + "snapshotId: {}.", snapshotID); + throw new NoSuchElementException(String.format("SnapshotId: %s is not " + + "found in snapshot chain.", snapshotID)); } - return snapshotChainGlobal - .get(snapshotID) - .getNextSnapshotID(); + return snapshotChainGlobal.get(snapshotID).getNextSnapshotID(); } /** @@ -375,19 +360,15 @@ public String nextGlobalSnapshot(String snapshotID) * @param snapshotID String, snapshot UUID * @return boolean */ - public boolean hasPreviousGlobalSnapshot(String snapshotID) - throws NoSuchElementException { - boolean hasPrevious = false; + public boolean hasPreviousGlobalSnapshot(String snapshotID) { if (!snapshotChainGlobal.containsKey(snapshotID)) { - LOG.error("no snapshot for provided snapshotID {}", snapshotID); - throw new NoSuchElementException("No snapshot: " + snapshotID); + LOG.error("No snapshot found in snapshot chain for provided " + + "snapshotId: {}.", snapshotID); + throw new NoSuchElementException(String.format("SnapshotId: %s is not " + + "found in snapshot chain.", snapshotID)); } - if (snapshotChainGlobal - .get(snapshotID) - .getPreviousSnapshotID() != null) { - hasPrevious = true; - } - return hasPrevious; + + return snapshotChainGlobal.get(snapshotID).getPreviousSnapshotID() != null; } /** @@ -396,17 +377,14 @@ public boolean hasPreviousGlobalSnapshot(String snapshotID) * @return String, snapshot UUID of previous snapshot in chain from * snapshotID */ - public String previousGlobalSnapshot(String snapshotID) - throws NoSuchElementException { + public String previousGlobalSnapshot(String snapshotID) { if (!hasPreviousGlobalSnapshot(snapshotID)) { - LOG.error("no preceeding snapshot for provided snapshotID {}", - snapshotID); - throw new NoSuchElementException("No preceeding snapshot from: " - + snapshotID); + LOG.error("No preceding snapshot found in snapshot chain for provided " + + "snapshotId: {}.", snapshotID); + throw new NoSuchElementException(String.format("SnapshotId: %s is not " + + "found in snapshot chain.", snapshotID)); } - return snapshotChainGlobal - .get(snapshotID) - .getPreviousSnapshotID(); + return snapshotChainGlobal.get(snapshotID).getPreviousSnapshotID(); } /** @@ -416,23 +394,20 @@ public String previousGlobalSnapshot(String snapshotID) * @param snapshotID String, snapshot UUID * @return boolean */ - public boolean hasNextPathSnapshot(String snapshotPath, String snapshotID) - throws NoSuchElementException { - boolean hasNext = false; + public boolean hasNextPathSnapshot(String snapshotPath, String snapshotID) { if (!snapshotChainPath.containsKey(snapshotPath) || !snapshotChainPath.get(snapshotPath).containsKey(snapshotID)) { - LOG.error("no snapshot for provided snapshotPath {} and " - + " snapshotID {}", snapshotPath, snapshotID); - throw new NoSuchElementException("No such snapshot: " - + snapshotID + "for path: " + snapshotPath); + LOG.error("No snapshot found for provided snapshotId: {} and " + + "snapshotPath: {}", snapshotID, snapshotPath); + throw new NoSuchElementException(String.format("SnapshotId: %s is not " + + "found in snapshot chain for snapshotPath: %s.", snapshotID, + snapshotPath)); } - if (snapshotChainPath + + return snapshotChainPath .get(snapshotPath) .get(snapshotID) - .getNextSnapshotID() != null) { - hasNext = true; - } - return hasNext; + .getNextSnapshotID() != null; } /** @@ -442,23 +417,19 @@ public boolean hasNextPathSnapshot(String snapshotPath, String snapshotID) * @param snapshotID String, snapshot UUID * @return boolean */ - public boolean hasPreviousPathSnapshot(String snapshotPath, String snapshotID) - throws NoSuchElementException { - boolean hasPrevious = false; + public boolean hasPreviousPathSnapshot(String snapshotPath, + String snapshotID) { if (!snapshotChainPath.containsKey(snapshotPath) || !snapshotChainPath.get(snapshotPath).containsKey(snapshotID)) { - LOG.error("no snapshot for provided snapshotPath {} and " - + " snapshotID {}", snapshotPath, snapshotID); - throw new NoSuchElementException("No snapshot: " + snapshotID - + " for snapshot path: " + snapshotPath); + LOG.error("No snapshot found for provided snapshotId: {} and " + + "snapshotPath: {}", snapshotID, snapshotPath); + throw new NoSuchElementException(String.format("SnapshotId: %s is not " + + "found in snapshot chain for snapshotPath: %s.", snapshotID, + snapshotPath)); } - if (snapshotChainPath - .get(snapshotPath) + return snapshotChainPath.get(snapshotPath) .get(snapshotID) - .getPreviousSnapshotID() != null) { - hasPrevious = true; - } - return hasPrevious; + .getPreviousSnapshotID() != null; } /** @@ -468,26 +439,24 @@ public boolean hasPreviousPathSnapshot(String snapshotPath, String snapshotID) * @return String, snapshot UUID of next snapshot in chain from * snapshotID */ - public String nextPathSnapshot(String snapshotPath, String snapshotID) - throws NoSuchElementException { + public String nextPathSnapshot(String snapshotPath, String snapshotID) { if (!snapshotChainPath.containsKey(snapshotPath) || !snapshotChainPath.get(snapshotPath).containsKey(snapshotID)) { - LOG.error("no snapshot for provided snapshotPath {} and " - + " snapshotID {}", snapshotPath, snapshotID); - throw new NoSuchElementException("No snapshot: " + snapshotID - + " for snapshot path:" + snapshotPath); + LOG.error("No snapshot found for provided snapshotId: {} and " + + "snapshotPath: {}", snapshotID, snapshotPath); + throw new NoSuchElementException(String.format("SnapshotId: %s is not " + + "found in snapshot chain for snapshotPath: %s.", snapshotID, + snapshotPath)); } - if (snapshotChainPath - .get(snapshotPath) - .get(snapshotID) + if (snapshotChainPath.get(snapshotPath).get(snapshotID) .getNextSnapshotID() == null) { - LOG.error("no following snapshot for provided snapshotPath {}, " - + "snapshotID {}", snapshotPath, snapshotID); - throw new NoSuchElementException("no following snapshot from: " - + snapshotID + "for snapshot path:" + snapshotPath); + LOG.error("No following snapshot for provided snapshotId {} and " + + "snapshotPath {}.", snapshotID, snapshotPath); + throw new NoSuchElementException(String.format("Ne following snapshot " + + "found in snapshot chain for snapshotId: %s and snapshotPath: " + + "%s.", snapshotID, snapshotPath)); } - return snapshotChainPath - .get(snapshotPath) + return snapshotChainPath.get(snapshotPath) .get(snapshotID) .getNextSnapshotID(); } @@ -499,23 +468,24 @@ public String nextPathSnapshot(String snapshotPath, String snapshotID) * @return String, snapshot UUID of previous snapshot in chain from * snapshotID */ - public String previousPathSnapshot(String snapshotPath, String snapshotID) - throws NoSuchElementException { + public String previousPathSnapshot(String snapshotPath, String snapshotID) { if (!snapshotChainPath.containsKey(snapshotPath) || !snapshotChainPath.get(snapshotPath).containsKey(snapshotID)) { - LOG.error("no snapshot for provided snapshotPath {} and " - + " snapshotID {}", snapshotPath, snapshotID); - throw new NoSuchElementException("No snapshot: " + snapshotID - + " for snapshot path:" + snapshotPath); + LOG.error("No snapshot found for provided snapshotId: {} and " + + "snapshotPath: {}", snapshotID, snapshotPath); + throw new NoSuchElementException(String.format("SnapshotId: %s is not " + + "found in snapshot chain for snapshotPath: %s.", snapshotID, + snapshotPath)); } if (snapshotChainPath .get(snapshotPath) .get(snapshotID) .getPreviousSnapshotID() == null) { - LOG.error("no preceeding snapshot for provided snapshotPath {}, " - + "snapshotID {}", snapshotPath, snapshotID); - throw new NoSuchElementException("no preceeding snapshot from: " - + snapshotID + "for snapshot path:" + snapshotPath); + LOG.error("No preceding snapshot for provided snapshotId: {} and " + + "snapshotPath: {}", snapshotPath, snapshotID); + throw new NoSuchElementException(String.format("Ne preceding snapshot " + + "found in snapshot chain for snapshotId: %s and snapshotPath: " + + "%s.", snapshotID, snapshotPath)); } return snapshotChainPath .get(snapshotPath) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java index 0bed12118d04..c92fa8f93910 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java @@ -45,6 +45,7 @@ import org.apache.hadoop.ozone.security.acl.IAccessAuthorizer; import org.apache.hadoop.ozone.security.acl.OzoneObj; import org.apache.hadoop.security.UserGroupInformation; +import org.apache.hadoop.util.Time; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -82,7 +83,8 @@ public OMSnapshotCreateRequest(OMRequest omRequest) { snapshotInfo = SnapshotInfo.newInstance(volumeName, bucketName, possibleName, - snapshotId); + snapshotId, + createSnapshotRequest.getCreationTime()); snapshotName = snapshotInfo.getName(); snapshotPath = snapshotInfo.getSnapshotPath(); } @@ -107,6 +109,7 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException { return omRequest.toBuilder().setCreateSnapshotRequest( omRequest.getCreateSnapshotRequest().toBuilder() .setSnapshotId(UUID.randomUUID().toString()) + .setCreationTime(Time.now()) .build()).build(); } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotDeleteRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotDeleteRequest.java index 33ec0876179f..cf2481d2c2b7 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotDeleteRequest.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotDeleteRequest.java @@ -208,7 +208,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, if (snapshotInfo == null) { // Dummy SnapshotInfo for logging and audit logging when erred snapshotInfo = SnapshotInfo.newInstance(volumeName, bucketName, - snapshotName, null); + snapshotName, null, Time.now()); } // Perform audit logging outside the lock diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotManager.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotManager.java index 6287cc00cbdb..3a8a694103eb 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotManager.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotManager.java @@ -28,6 +28,7 @@ import org.apache.hadoop.hdds.utils.db.Table; import org.apache.hadoop.ozone.om.helpers.SnapshotInfo; import org.apache.hadoop.ozone.om.snapshot.OmSnapshotUtils; +import org.apache.hadoop.util.Time; import org.apache.ozone.test.GenericTestUtils; import org.junit.After; import org.junit.Assert; @@ -196,7 +197,8 @@ private SnapshotInfo createSnapshotInfo() { return SnapshotInfo.newInstance(volumeName, bucketName, snapshotName, - snapshotId); + snapshotId, + Time.now()); } } diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/OMRequestTestUtils.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/OMRequestTestUtils.java index ea687d1fed0c..40ae214b7ebe 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/OMRequestTestUtils.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/OMRequestTestUtils.java @@ -365,7 +365,7 @@ public static void addSnapshotToTable( String volumeName, String bucketName, String snapshotName, OMMetadataManager omMetadataManager) throws IOException { SnapshotInfo snapshotInfo = SnapshotInfo.newInstance(volumeName, - bucketName, snapshotName, UUID.randomUUID().toString()); + bucketName, snapshotName, UUID.randomUUID().toString(), Time.now()); addSnapshotToTable(false, 0L, snapshotInfo, omMetadataManager); } @@ -376,7 +376,7 @@ public static void addSnapshotToTableCache( String volumeName, String bucketName, String snapshotName, OMMetadataManager omMetadataManager) throws IOException { SnapshotInfo snapshotInfo = SnapshotInfo.newInstance(volumeName, bucketName, - snapshotName, UUID.randomUUID().toString()); + snapshotName, UUID.randomUUID().toString(), Time.now()); addSnapshotToTable(true, 0L, snapshotInfo, omMetadataManager); } diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotDeleteRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotDeleteRequest.java index ae28a9cd3152..e09eb26806dd 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotDeleteRequest.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/snapshot/TestOMSnapshotDeleteRequest.java @@ -41,6 +41,7 @@ import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMResponse; import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Status; import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Type; +import org.apache.hadoop.util.Time; import org.apache.ozone.test.LambdaTestUtils; import org.junit.After; import org.junit.Assert; @@ -206,7 +207,7 @@ public void testValidateAndUpdateCache() throws Exception { // add key to cache SnapshotInfo snapshotInfo = SnapshotInfo.newInstance( - volumeName, bucketName, snapshotName, null); + volumeName, bucketName, snapshotName, null, Time.now()); Assert.assertEquals(SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE, snapshotInfo.getSnapshotStatus()); omMetadataManager.getSnapshotInfoTable().addCacheEntry( diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/snapshot/TestOMSnapshotCreateResponse.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/snapshot/TestOMSnapshotCreateResponse.java index f8a67fbe96f1..3376717c9835 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/snapshot/TestOMSnapshotCreateResponse.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/snapshot/TestOMSnapshotCreateResponse.java @@ -30,6 +30,7 @@ import org.apache.hadoop.hdds.utils.db.TableIterator; import org.apache.hadoop.ozone.om.helpers.RepeatedOmKeyInfo; import org.apache.hadoop.ozone.om.helpers.SnapshotInfo; +import org.apache.hadoop.util.Time; import org.junit.After; import org.junit.Assert; import org.junit.Before; @@ -86,7 +87,8 @@ public void testAddToDBBatch() throws Exception { SnapshotInfo snapshotInfo = SnapshotInfo.newInstance(volumeName, bucketName, snapshotName, - snapshotId); + snapshotId, + Time.now()); // confirm table is empty Assert.assertEquals(0, diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/snapshot/TestOMSnapshotDeleteResponse.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/snapshot/TestOMSnapshotDeleteResponse.java index bfbeeafb2bbb..39eb8a7cf752 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/snapshot/TestOMSnapshotDeleteResponse.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/response/snapshot/TestOMSnapshotDeleteResponse.java @@ -34,6 +34,7 @@ import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMResponse; import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Status; import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Type; +import org.apache.hadoop.util.Time; import org.junit.After; import org.junit.Assert; import org.junit.Before; @@ -86,7 +87,8 @@ public void testAddToDBBatch() throws Exception { SnapshotInfo snapshotInfo = SnapshotInfo.newInstance(volumeName, bucketName, snapshotName, - snapshotId); + snapshotId, + Time.now()); // confirm table is empty Assert.assertEquals(0, From c10f011808740c7b457496e4c41266bec5f9976c Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Tue, 16 May 2023 19:29:11 -0700 Subject: [PATCH 2/3] Addressed review comments --- .../org/apache/hadoop/ozone/om/TestOzoneManagerHASnapshot.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHASnapshot.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHASnapshot.java index a44fd20dac3a..3f0267e84510 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHASnapshot.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHASnapshot.java @@ -48,7 +48,7 @@ public class TestOzoneManagerHASnapshot extends TestOzoneManagerHA { * passed or empty. */ @Test - public void testUniqueSnapshotName() throws Exception { + public void testSnapshotNameConsistency() throws Exception { OzoneBucket ozoneBucket = setupBucket(); String volumeName = ozoneBucket.getVolumeName(); String bucketName = ozoneBucket.getName(); @@ -116,5 +116,6 @@ public void testSnapshotChainManagerRestore() throws Exception { await().atMost(Duration.ofSeconds(180)) .until(() -> getCluster().getOMLeader() != null); + assertNotNull(getCluster().getOMLeader()); } } From b02fc5c8f26a6c7bd8009234798b41a46b2e4551 Mon Sep 17 00:00:00 2001 From: Hemant Kumar Date: Wed, 17 May 2023 15:25:46 -0700 Subject: [PATCH 3/3] Apply suggestions from code review Co-authored-by: Aswin Shakil Balasubramanian --- .../org/apache/hadoop/ozone/om/SnapshotChainManager.java | 6 +++--- 1 file changed, 3 insertions(+), 3 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 ef78434f8b99..c3ec6d1d610c 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 @@ -452,7 +452,7 @@ public String nextPathSnapshot(String snapshotPath, String snapshotID) { .getNextSnapshotID() == null) { LOG.error("No following snapshot for provided snapshotId {} and " + "snapshotPath {}.", snapshotID, snapshotPath); - throw new NoSuchElementException(String.format("Ne following snapshot " + + throw new NoSuchElementException(String.format("No following snapshot " + "found in snapshot chain for snapshotId: %s and snapshotPath: " + "%s.", snapshotID, snapshotPath)); } @@ -482,8 +482,8 @@ public String previousPathSnapshot(String snapshotPath, String snapshotID) { .get(snapshotID) .getPreviousSnapshotID() == null) { LOG.error("No preceding snapshot for provided snapshotId: {} and " + - "snapshotPath: {}", snapshotPath, snapshotID); - throw new NoSuchElementException(String.format("Ne preceding snapshot " + + "snapshotPath: {}", snapshotID, snapshotPath); + throw new NoSuchElementException(String.format("No preceding snapshot " + "found in snapshot chain for snapshotId: %s and snapshotPath: " + "%s.", snapshotID, snapshotPath)); }