From 22c634f3fe75c768256ca14304d1a3b7b3711900 Mon Sep 17 00:00:00 2001 From: Siyao Meng <50227127+smengcl@users.noreply.github.com> Date: Tue, 3 Jun 2025 10:53:16 -0700 Subject: [PATCH 01/20] HDDS-13006. Use yaml files to host Ozone snapshot local properties --- .../om/OmSnapshotLocalPropertyManager.java | 88 +++++++++++++++++++ .../hadoop/ozone/om/OmSnapshotManager.java | 5 ++ 2 files changed, 93 insertions(+) create mode 100644 hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyManager.java diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyManager.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyManager.java new file mode 100644 index 000000000000..88b805be8c22 --- /dev/null +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyManager.java @@ -0,0 +1,88 @@ +/* + * 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.hadoop.ozone.om.exceptions.OMException; + +import java.io.IOException; +import java.util.Map; + +/** + * Interface to manage Ozone snapshot DB local checkpoint metadata properties. + * Those properties are per-OM, e.g. isSSTFiltered flag, which differs from the ones stored in SnapshotInfo proto. + */ +public interface OmSnapshotLocalPropertyManager { + + /** + * Sets a property for a snapshot. + * + * @param yamlFilePath Path to the snapshot's YAML property file + * @param key Property key + * @param value Property value + * @throws IOException if an I/O error occurs + * @throws OMException if snapshot doesn't exist or operation fails + */ + void setProperty(String yamlFilePath, String key, String value) + throws IOException, OMException; + + /** + * Gets a property value for a snapshot. + * + * @param yamlFilePath Path to the snapshot's YAML property file + * @param key Property key + * @return Property value or null if not found + * @throws IOException if an I/O error occurs + * @throws OMException if snapshot doesn't exist or operation fails + */ + String getProperty(String yamlFilePath, String key) + throws IOException, OMException; + + /** + * Gets all properties for a snapshot. + * + * @param yamlFilePath Path to the snapshot's YAML property file + * @return Map of property key-value pairs + * @throws IOException if an I/O error occurs + * @throws OMException if snapshot doesn't exist or operation fails + */ + Map getProperties(String yamlFilePath) + throws IOException, OMException; + + /** + * Checks if a property exists for a snapshot. + * + * @param yamlFilePath Path to the snapshot's YAML property file + * @param key Property key + * @return true if the property exists, false otherwise + * @throws IOException if an I/O error occurs + * @throws OMException if snapshot doesn't exist or operation fails + */ + boolean hasProperty(String yamlFilePath, String key) + throws IOException, OMException; + + /** + * Removes a property from a snapshot. + * + * @param yamlFilePath Path to the snapshot's YAML property file + * @param key Property key + * @throws IOException if an I/O error occurs + * @throws OMException if snapshot doesn't exist or operation fails + */ + void removeProperty(String yamlFilePath, String key) + throws IOException, OMException; +} 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 262750e5c2f2..84d5e65c4228 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 @@ -764,6 +764,11 @@ public static String getSnapshotPath(OzoneConfiguration conf, OM_DB_NAME + snapshotInfo.getCheckpointDirName(); } + public static String getSnapshotLocalPropertyPath(OzoneConfiguration conf, + SnapshotInfo snapshotInfo) { + return getSnapshotPath(conf, snapshotInfo) + ".yaml"; + } + public static boolean isSnapshotKey(String[] keyParts) { return (keyParts.length > 1) && (keyParts[0].compareTo(OM_SNAPSHOT_INDICATOR) == 0); From 03bc93d0040196b95ed35fb7fcfc8aa72a90557c Mon Sep 17 00:00:00 2001 From: Siyao Meng <50227127+smengcl@users.noreply.github.com> Date: Sat, 7 Jun 2025 21:44:19 -0700 Subject: [PATCH 02/20] Add implementation `OmSnapshotLocalPropertyYamlImpl`. Generated-by: Claude 3.7 Sonnet (then manually reviewed and tweaked) --- ...ager.java => OmSnapshotLocalProperty.java} | 35 ++-- .../om/OmSnapshotLocalPropertyYamlImpl.java | 176 ++++++++++++++++++ .../hadoop/ozone/om/OmSnapshotManager.java | 2 +- 3 files changed, 189 insertions(+), 24 deletions(-) rename hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/{OmSnapshotLocalPropertyManager.java => OmSnapshotLocalProperty.java} (68%) create mode 100644 hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyYamlImpl.java diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyManager.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalProperty.java similarity index 68% rename from hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyManager.java rename to hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalProperty.java index 88b805be8c22..a163f9254302 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyManager.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalProperty.java @@ -17,72 +17,61 @@ package org.apache.hadoop.ozone.om; -import org.apache.hadoop.ozone.om.exceptions.OMException; - import java.io.IOException; import java.util.Map; +import org.apache.hadoop.ozone.om.exceptions.OMException; /** * Interface to manage Ozone snapshot DB local checkpoint metadata properties. * Those properties are per-OM, e.g. isSSTFiltered flag, which differs from the ones stored in SnapshotInfo proto. */ -public interface OmSnapshotLocalPropertyManager { +public interface OmSnapshotLocalProperty extends AutoCloseable { /** * Sets a property for a snapshot. * - * @param yamlFilePath Path to the snapshot's YAML property file - * @param key Property key - * @param value Property value + * @param key Property key + * @param value Property value * @throws IOException if an I/O error occurs * @throws OMException if snapshot doesn't exist or operation fails */ - void setProperty(String yamlFilePath, String key, String value) - throws IOException, OMException; + void setProperty(String key, String value) throws IOException; /** * Gets a property value for a snapshot. * - * @param yamlFilePath Path to the snapshot's YAML property file - * @param key Property key + * @param key Property key * @return Property value or null if not found * @throws IOException if an I/O error occurs * @throws OMException if snapshot doesn't exist or operation fails */ - String getProperty(String yamlFilePath, String key) - throws IOException, OMException; + String getProperty(String key) throws IOException; /** * Gets all properties for a snapshot. * - * @param yamlFilePath Path to the snapshot's YAML property file * @return Map of property key-value pairs * @throws IOException if an I/O error occurs * @throws OMException if snapshot doesn't exist or operation fails */ - Map getProperties(String yamlFilePath) - throws IOException, OMException; + Map getProperties() throws IOException; /** * Checks if a property exists for a snapshot. * - * @param yamlFilePath Path to the snapshot's YAML property file - * @param key Property key + * @param key Property key * @return true if the property exists, false otherwise * @throws IOException if an I/O error occurs * @throws OMException if snapshot doesn't exist or operation fails */ - boolean hasProperty(String yamlFilePath, String key) - throws IOException, OMException; + boolean hasProperty(String key) throws IOException; /** * Removes a property from a snapshot. * - * @param yamlFilePath Path to the snapshot's YAML property file - * @param key Property key + * @param key Property key * @throws IOException if an I/O error occurs * @throws OMException if snapshot doesn't exist or operation fails */ - void removeProperty(String yamlFilePath, String key) - throws IOException, OMException; + void removeProperty(String key) throws IOException; } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyYamlImpl.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyYamlImpl.java new file mode 100644 index 000000000000..bcdfbd559aa9 --- /dev/null +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyYamlImpl.java @@ -0,0 +1,176 @@ +/* + * 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 java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; +import java.util.HashMap; +import java.util.Map; +import java.util.Objects; +import java.util.concurrent.atomic.AtomicBoolean; +import org.apache.hadoop.hdds.server.YamlUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.yaml.snakeyaml.DumperOptions; +import org.yaml.snakeyaml.Yaml; + +/** + * Implementation of {@link OmSnapshotLocalProperty} that uses a YAML file + * to store and retrieve snapshot local properties. + * Changes are only persisted when the object is closed. + */ +public class OmSnapshotLocalPropertyYamlImpl implements OmSnapshotLocalProperty, AutoCloseable { + + private static final Logger LOG = LoggerFactory.getLogger(OmSnapshotLocalPropertyYamlImpl.class); + + /** + * The path to the YAML file used to store properties. + * If this file does not exist, it will be created upon close(). + */ + private final File yamlFile; + + /** + * A map storing key-value pairs of snapshot properties. + * Read from the YAML file upon initialization. + */ + private Map properties; + + /** + * Flag indicating whether the properties have been modified + * since the last save to the YAML file. + * Used to determine if file write is needed on close. + */ + private final AtomicBoolean isDirty = new AtomicBoolean(false); + + /** + * Flag indicating whether this instance has been closed. + * Operations attempted after closing will throw an OMException. + */ + private final AtomicBoolean isClosed = new AtomicBoolean(false); + + /** + * Constructs a new OmSnapshotLocalPropertyYamlImpl. + * + * @param yamlFilePath Path to the YAML file + */ + public OmSnapshotLocalPropertyYamlImpl(String yamlFilePath) throws IOException { + this.yamlFile = new File(yamlFilePath); + loadPropertiesFromFile(); + } + + @Override + public void setProperty(String key, String value) throws IOException { + checkIfClosed(); + String oldValue = properties.get(key); + if (!Objects.equals(value, oldValue)) { + properties.put(key, value); + isDirty.set(true); + } + } + + @Override + public String getProperty(String key) throws IOException { + checkIfClosed(); + return properties.get(key); + } + + @Override + public boolean hasProperty(String key) throws IOException { + checkIfClosed(); + return properties.containsKey(key); + } + + @Override + public void removeProperty(String key) throws IOException { + checkIfClosed(); + if (properties.containsKey(key)) { + properties.remove(key); + isDirty.set(true); + } + } + + @Override + public Map getProperties() throws IOException { + checkIfClosed(); + return new HashMap<>(properties); + } + + /** + * Saves any pending changes to the YAML file and releases resources. + * + * @throws IOException if an I/O error occurs saving the file + */ + @Override + public void close() throws IOException { + if (isClosed.compareAndSet(false, true)) { + if (isDirty.get()) { + LOG.debug("Saving changes to properties file: {}", yamlFile); + savePropertiesToFile(); + } + } + } + + /** + * Checks if the object has been closed. + * + * @throws IOException if the object has been closed + */ + private void checkIfClosed() throws IOException { + if (isClosed.get()) { + throw new IOException("OmSnapshotLocalPropertyYamlImpl has been closed"); + } + } + + /** + * Loads the properties from the YAML file. + * + * @throws IOException if an I/O error occurs, or if the YAML file is not properly formatted + */ + private void loadPropertiesFromFile() throws IOException { + if (!yamlFile.exists()) { + LOG.debug("YAML file does not exist, creating empty properties map"); + properties = new HashMap<>(); + return; + } + + try (InputStream inputStream = Files.newInputStream(yamlFile.toPath())) { + Map loadedProperties = YamlUtils.loadAs(inputStream, Map.class); + properties = loadedProperties != null ? new HashMap<>(loadedProperties) : new HashMap<>(); + } catch (IOException e) { + LOG.error("Unable to parse YAML file: {}", yamlFile, e); + throw new IOException("Unable to parse snapshot properties YAML file", e); + } + } + + /** + * Saves the properties to the YAML file. + * + * @throws IOException if an I/O error occurs + */ + private void savePropertiesToFile() throws IOException { + DumperOptions options = new DumperOptions(); + options.setPrettyFlow(true); + options.setDefaultFlowStyle(DumperOptions.FlowStyle.BLOCK); + Yaml yaml = new Yaml(options); + + YamlUtils.dump(yaml, properties, yamlFile, LOG); + isDirty.set(false); + } +} 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 84d5e65c4228..ec1b06758f66 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 @@ -764,7 +764,7 @@ public static String getSnapshotPath(OzoneConfiguration conf, OM_DB_NAME + snapshotInfo.getCheckpointDirName(); } - public static String getSnapshotLocalPropertyPath(OzoneConfiguration conf, + public static String getSnapshotLocalPropertyYamlPath(OzoneConfiguration conf, SnapshotInfo snapshotInfo) { return getSnapshotPath(conf, snapshotInfo) + ".yaml"; } From f7eb655cc61d6d7b2b490ff200104b51fef78443 Mon Sep 17 00:00:00 2001 From: Siyao Meng <50227127+smengcl@users.noreply.github.com> Date: Sat, 7 Jun 2025 23:02:55 -0700 Subject: [PATCH 03/20] Modify `SstFilteringService` to use `OmSnapshotLocalPropertyYamlImpl`. Generated-by: Claude 3.7 Sonnet (then manually reviewed and tweaked) --- .../hadoop/ozone/om/SstFilteringService.java | 33 ++++++++++++++++--- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java index ea46366d9182..89be2253422d 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java @@ -71,6 +71,7 @@ public class SstFilteringService extends BackgroundService private static final int SST_FILTERING_CORE_POOL_SIZE = 1; public static final String SST_FILTERED_FILE = "sstFiltered"; + public static final String SST_FILTERED_YAML_KEY = "sstFiltered"; private static final byte[] SST_FILTERED_FILE_CONTENT = StringUtils.string2Bytes("This file holds information " + "if a particular snapshot has filtered out the relevant sst files or not.\nDO NOT add, change or delete " + "any files in this directory unless you know what you are doing.\n"); @@ -86,8 +87,20 @@ public class SstFilteringService extends BackgroundService private final BootstrapStateHandler.Lock lock = new BootstrapStateHandler.Lock(); public static boolean isSstFiltered(OzoneConfiguration ozoneConfiguration, SnapshotInfo snapshotInfo) { - Path sstFilteredFile = Paths.get(OmSnapshotManager.getSnapshotPath(ozoneConfiguration, - snapshotInfo), SST_FILTERED_FILE); + // First try to read the flag from YAML file + String yamlPath = OmSnapshotManager.getSnapshotLocalPropertyYamlPath(ozoneConfiguration, snapshotInfo); + + try (OmSnapshotLocalProperty localProperties = new OmSnapshotLocalPropertyYamlImpl(yamlPath)) { + String sstFilteredProperty = localProperties.getProperty(SST_FILTERED_YAML_KEY); + return Boolean.parseBoolean(sstFilteredProperty); + } catch (Exception e) { + // If we can't read the YAML file, fall back to the existing checks + LOG.debug("Failed to read snapshot local properties from YAML file: {}", yamlPath, e); + } + + // Fall back to existing checks + Path sstFilteredFile = Paths.get( + OmSnapshotManager.getSnapshotPath(ozoneConfiguration, snapshotInfo), SST_FILTERED_FILE); return snapshotInfo.isSstFiltered() || sstFilteredFile.toFile().exists(); } @@ -119,14 +132,14 @@ public void resume() { running.set(true); } - private class SstFilteringTask implements BackgroundTask { + private final class SstFilteringTask implements BackgroundTask { private boolean isSnapshotDeleted(SnapshotInfo snapshotInfo) { return snapshotInfo == null || snapshotInfo.getSnapshotStatus() == SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED; } /** - * Marks the snapshot as SSTFiltered by creating a file in snapshot directory. + * Marks the snapshot as SSTFiltered. * @param snapshotInfo snapshotInfo * @throws IOException */ @@ -141,8 +154,18 @@ private void markSSTFilteredFlagForSnapshot(SnapshotInfo snapshotInfo) throws IO if (acquiredSnapshotLock) { String snapshotDir = OmSnapshotManager.getSnapshotPath(ozoneManager.getConfiguration(), snapshotInfo); try { - // mark the snapshot as filtered by creating a file. + // Mark the snapshot as filtered by writing to YAML property file if (Files.exists(Paths.get(snapshotDir))) { + String yamlPath = OmSnapshotManager.getSnapshotLocalPropertyYamlPath( + ozoneManager.getConfiguration(), snapshotInfo); + try (OmSnapshotLocalProperty localProperties = new OmSnapshotLocalPropertyYamlImpl(yamlPath)) { + localProperties.setProperty(SST_FILTERED_YAML_KEY, "true"); + } catch (Exception e) { + LOG.error("Failed to set SST filtered local property for snapshot: {}", snapshotInfo.getName(), e); + } + + // For backward compatibility, still create the touch file (e.g. when upgraded but not finalized yet) + // TODO: When upgrade is finalized, this can be skipped Files.write(Paths.get(snapshotDir, SST_FILTERED_FILE), SST_FILTERED_FILE_CONTENT); } } finally { From b9554a8cbd55c667c743836c79a199dd14819967 Mon Sep 17 00:00:00 2001 From: Siyao Meng <50227127+smengcl@users.noreply.github.com> Date: Sat, 7 Jun 2025 23:06:07 -0700 Subject: [PATCH 04/20] Add unit tests `TestOmSnapshotLocalPropertyYamlImpl` for `OmSnapshotLocalPropertyYamlImpl`. Generated-by: Claude 3.7 Sonnet (then manually reviewed and tweaked) --- .../TestOmSnapshotLocalPropertyYamlImpl.java | 264 ++++++++++++++++++ 1 file changed, 264 insertions(+) create mode 100644 hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalPropertyYamlImpl.java diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalPropertyYamlImpl.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalPropertyYamlImpl.java new file mode 100644 index 000000000000..710ce03261ab --- /dev/null +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalPropertyYamlImpl.java @@ -0,0 +1,264 @@ +/* + * 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 static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.File; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.HashMap; +import java.util.Map; +import org.apache.hadoop.hdds.server.YamlUtils; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.yaml.snakeyaml.DumperOptions; +import org.yaml.snakeyaml.Yaml; + +/** + * Tests for {@link OmSnapshotLocalPropertyYamlImpl}. + */ +public class TestOmSnapshotLocalPropertyYamlImpl { + + private static final Logger LOG = LoggerFactory.getLogger(TestOmSnapshotLocalPropertyYamlImpl.class); + + @TempDir + private Path tempDir; + + private File yamlFile; + private OmSnapshotLocalPropertyYamlImpl propertyImpl; + + @BeforeEach + public void setUp() throws IOException { + yamlFile = tempDir.resolve("test-properties.yaml").toFile(); + propertyImpl = new OmSnapshotLocalPropertyYamlImpl(yamlFile.getAbsolutePath()); + } + + @AfterEach + public void tearDown() throws IOException { + if (propertyImpl != null) { + propertyImpl.close(); + } + + if (yamlFile.exists()) { + yamlFile.delete(); + } + } + + @Test + public void testInitWithNonExistentFile() throws IOException { + // Verify that the implementation carries an empty properties map when the YAML file doesn't exist + Map properties = propertyImpl.getProperties(); + assertNotNull(properties); + assertTrue(properties.isEmpty()); + + // Now set a property and close to create the file + String key = "testKey"; + String value = "testValue"; + propertyImpl.setProperty(key, value); + + // File should not be created until close is called + assertFalse(yamlFile.exists(), "YAML file should not exist before close"); + + propertyImpl.close(); + assertTrue(yamlFile.exists(), "YAML file should be created on close"); + } + + @Test + public void testSetAndGetProperty() throws IOException { + // Set a property + String key = "testKey"; + String value = "testValue"; + propertyImpl.setProperty(key, value); + + // Verify the property was set in memory + assertEquals(value, propertyImpl.getProperty(key)); + assertTrue(propertyImpl.hasProperty(key)); + + // Verify the file is created with correct content after close + propertyImpl.close(); + assertTrue(yamlFile.exists()); + + // Read back for verification + try (OmSnapshotLocalPropertyYamlImpl newPropertyImpl = + new OmSnapshotLocalPropertyYamlImpl(yamlFile.getAbsolutePath())) { + assertEquals(value, newPropertyImpl.getProperty(key)); + } + } + + @Test + public void testSetPropertyNoWriteUponCloseWhenNotDirty() throws IOException { + // Setting the same property with the same value should not mark as dirty + String key = "testKey"; + String value = "testValue"; + + propertyImpl.setProperty(key, value); + propertyImpl.close(); + + // close() should create the file if it doesn't exist in this case + assertTrue(yamlFile.exists()); + + // Store the last modification time + long lastModified = yamlFile.lastModified(); + + propertyImpl = new OmSnapshotLocalPropertyYamlImpl(yamlFile.getAbsolutePath()); + // Same key-value, should not be dirty + propertyImpl.setProperty(key, value); + propertyImpl.close(); + + // File modification time should not have changed since nothing changed + assertEquals(lastModified, yamlFile.lastModified(), + "File should not have been modified since properties didn't change"); + } + + @Test + public void testUpdateProperty() throws IOException { + // Set and then update a property + String key = "testKey"; + String originalValue = "originalValue"; + String updatedValue = "updatedValue"; + + propertyImpl.setProperty(key, originalValue); + assertEquals(originalValue, propertyImpl.getProperty(key)); + + propertyImpl.setProperty(key, updatedValue); + assertEquals(updatedValue, propertyImpl.getProperty(key)); + + propertyImpl.close(); + + // Verify updated value is persisted + try (OmSnapshotLocalPropertyYamlImpl newPropertyImpl = + new OmSnapshotLocalPropertyYamlImpl(yamlFile.getAbsolutePath())) { + assertEquals(updatedValue, newPropertyImpl.getProperty(key)); + } + } + + @Test + public void testRemoveProperty() throws IOException { + // Set and then remove a property + String key = "testKey"; + String value = "testValue"; + + propertyImpl.setProperty(key, value); + assertTrue(propertyImpl.hasProperty(key)); + + propertyImpl.removeProperty(key); + assertFalse(propertyImpl.hasProperty(key)); + assertNull(propertyImpl.getProperty(key)); + + propertyImpl.close(); + + // Verify property removal is persisted + try (OmSnapshotLocalPropertyYamlImpl newPropertyImpl = + new OmSnapshotLocalPropertyYamlImpl(yamlFile.getAbsolutePath())) { + assertFalse(newPropertyImpl.hasProperty(key)); + } + } + + @Test + public void testRemoveNonExistentProperty() throws IOException { + // Removing a non-existent property should not cause errors + propertyImpl.removeProperty("nonExistentKey"); + + // Should not mark as dirty, so file shouldn't be created on close + propertyImpl.close(); + assertFalse(yamlFile.exists()); + } + + @Test + public void testGetProperties() throws IOException { + // Set multiple properties + Map testProperties = new HashMap<>(); + testProperties.put("key1", "value1"); + testProperties.put("key2", "value2"); + testProperties.put("key3", "value3"); + + for (Map.Entry entry : testProperties.entrySet()) { + propertyImpl.setProperty(entry.getKey(), entry.getValue()); + } + + // Verify all properties are accessible + Map retrievedProperties = propertyImpl.getProperties(); + assertEquals(testProperties.size(), retrievedProperties.size()); + for (Map.Entry entry : testProperties.entrySet()) { + assertEquals(entry.getValue(), retrievedProperties.get(entry.getKey())); + } + + // Verify modifications to returned map don't affect internal state + retrievedProperties.put("newKey", "newValue"); + assertFalse(propertyImpl.hasProperty("newKey")); + } + + @Test + public void testLoadFromExistingFile() throws IOException { + // Create a file with predefined properties + Map initialProperties = new HashMap<>(); + initialProperties.put("existing1", "value1"); + initialProperties.put("existing2", "value2"); + + DumperOptions options = new DumperOptions(); + options.setPrettyFlow(true); + options.setDefaultFlowStyle(DumperOptions.FlowStyle.FLOW); + Yaml yaml = new Yaml(options); + + YamlUtils.dump(yaml, initialProperties, yamlFile, LOG); + + // Instantiate with existing file + try (OmSnapshotLocalPropertyYamlImpl loadedPropertyImpl = + new OmSnapshotLocalPropertyYamlImpl(yamlFile.getAbsolutePath())) { + // Verify properties are loaded correctly + assertEquals("value1", loadedPropertyImpl.getProperty("existing1")); + assertEquals("value2", loadedPropertyImpl.getProperty("existing2")); + assertEquals(2, loadedPropertyImpl.getProperties().size()); + } + } + + @Test + public void testOperationsAfterClose() throws IOException { + propertyImpl.close(); + + // All operations should throw IOException after close + assertThrows(IOException.class, () -> propertyImpl.getProperty("anyKey")); + assertThrows(IOException.class, () -> propertyImpl.setProperty("anyKey", "anyValue")); + assertThrows(IOException.class, () -> propertyImpl.hasProperty("anyKey")); + assertThrows(IOException.class, () -> propertyImpl.removeProperty("anyKey")); + assertThrows(IOException.class, () -> propertyImpl.getProperties()); + } + + @Test + public void testInvalidYamlFile() throws IOException { + // Create file with invalid YAML content + Files.write(yamlFile.toPath(), "invalid: yaml: content: - not properly formatted".getBytes()); + + // Should throw IOException with appropriate message + IOException exception = assertThrows(IOException.class, + () -> new OmSnapshotLocalPropertyYamlImpl(yamlFile.getAbsolutePath())); + + assertTrue(exception.getMessage().contains("Unable to parse snapshot properties YAML file")); + } +} From 56afd4e3194604a03aee1d2146165b80774afaf6 Mon Sep 17 00:00:00 2001 From: Siyao Meng <50227127+smengcl@users.noreply.github.com> Date: Sat, 7 Jun 2025 23:32:53 -0700 Subject: [PATCH 05/20] Tweak Impl --- .../hadoop/ozone/om/OmSnapshotLocalPropertyYamlImpl.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyYamlImpl.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyYamlImpl.java index bcdfbd559aa9..1c967a9b05c4 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyYamlImpl.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyYamlImpl.java @@ -30,6 +30,7 @@ import org.slf4j.LoggerFactory; import org.yaml.snakeyaml.DumperOptions; import org.yaml.snakeyaml.Yaml; +import org.yaml.snakeyaml.scanner.ScannerException; /** * Implementation of {@link OmSnapshotLocalProperty} that uses a YAML file @@ -153,7 +154,7 @@ private void loadPropertiesFromFile() throws IOException { try (InputStream inputStream = Files.newInputStream(yamlFile.toPath())) { Map loadedProperties = YamlUtils.loadAs(inputStream, Map.class); properties = loadedProperties != null ? new HashMap<>(loadedProperties) : new HashMap<>(); - } catch (IOException e) { + } catch (IOException | ScannerException e) { LOG.error("Unable to parse YAML file: {}", yamlFile, e); throw new IOException("Unable to parse snapshot properties YAML file", e); } @@ -167,7 +168,7 @@ private void loadPropertiesFromFile() throws IOException { private void savePropertiesToFile() throws IOException { DumperOptions options = new DumperOptions(); options.setPrettyFlow(true); - options.setDefaultFlowStyle(DumperOptions.FlowStyle.BLOCK); + options.setDefaultFlowStyle(DumperOptions.FlowStyle.FLOW); Yaml yaml = new Yaml(options); YamlUtils.dump(yaml, properties, yamlFile, LOG); From 08a90ff7364b62d814e56570f2fe135da234119a Mon Sep 17 00:00:00 2001 From: Siyao Meng <50227127+smengcl@users.noreply.github.com> Date: Sat, 7 Jun 2025 23:39:49 -0700 Subject: [PATCH 06/20] Tweak doc --- .../org/apache/hadoop/ozone/om/OmSnapshotLocalProperty.java | 6 ------ 1 file changed, 6 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalProperty.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalProperty.java index a163f9254302..1c87d6639933 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalProperty.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalProperty.java @@ -19,7 +19,6 @@ import java.io.IOException; import java.util.Map; -import org.apache.hadoop.ozone.om.exceptions.OMException; /** * Interface to manage Ozone snapshot DB local checkpoint metadata properties. @@ -33,7 +32,6 @@ public interface OmSnapshotLocalProperty extends AutoCloseable { * @param key Property key * @param value Property value * @throws IOException if an I/O error occurs - * @throws OMException if snapshot doesn't exist or operation fails */ void setProperty(String key, String value) throws IOException; @@ -43,7 +41,6 @@ public interface OmSnapshotLocalProperty extends AutoCloseable { * @param key Property key * @return Property value or null if not found * @throws IOException if an I/O error occurs - * @throws OMException if snapshot doesn't exist or operation fails */ String getProperty(String key) throws IOException; @@ -52,7 +49,6 @@ public interface OmSnapshotLocalProperty extends AutoCloseable { * * @return Map of property key-value pairs * @throws IOException if an I/O error occurs - * @throws OMException if snapshot doesn't exist or operation fails */ Map getProperties() throws IOException; @@ -62,7 +58,6 @@ public interface OmSnapshotLocalProperty extends AutoCloseable { * @param key Property key * @return true if the property exists, false otherwise * @throws IOException if an I/O error occurs - * @throws OMException if snapshot doesn't exist or operation fails */ boolean hasProperty(String key) throws IOException; @@ -71,7 +66,6 @@ public interface OmSnapshotLocalProperty extends AutoCloseable { * * @param key Property key * @throws IOException if an I/O error occurs - * @throws OMException if snapshot doesn't exist or operation fails */ void removeProperty(String key) throws IOException; } From f5a46a88365dd31edb3ed371a7b736c085876d61 Mon Sep 17 00:00:00 2001 From: Siyao Meng <50227127+smengcl@users.noreply.github.com> Date: Sat, 7 Jun 2025 23:42:44 -0700 Subject: [PATCH 07/20] Make `getProperties()` return immutable map. --- .../hadoop/ozone/om/OmSnapshotLocalPropertyYamlImpl.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyYamlImpl.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyYamlImpl.java index 1c967a9b05c4..afefb3954b8f 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyYamlImpl.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyYamlImpl.java @@ -21,6 +21,7 @@ import java.io.IOException; import java.io.InputStream; import java.nio.file.Files; +import java.util.Collections; import java.util.HashMap; import java.util.Map; import java.util.Objects; @@ -110,7 +111,7 @@ public void removeProperty(String key) throws IOException { @Override public Map getProperties() throws IOException { checkIfClosed(); - return new HashMap<>(properties); + return Collections.unmodifiableMap(properties); } /** From b86888b3ea282ad16f0c5a17cbecae646599d978 Mon Sep 17 00:00:00 2001 From: Siyao Meng <50227127+smengcl@users.noreply.github.com> Date: Sat, 7 Jun 2025 23:51:24 -0700 Subject: [PATCH 08/20] Cleanup --- .../hadoop/ozone/om/OmSnapshotLocalPropertyYamlImpl.java | 4 ++-- .../ozone/om/TestOmSnapshotLocalPropertyYamlImpl.java | 9 ++------- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyYamlImpl.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyYamlImpl.java index afefb3954b8f..5035ddb160b3 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyYamlImpl.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyYamlImpl.java @@ -156,8 +156,8 @@ private void loadPropertiesFromFile() throws IOException { Map loadedProperties = YamlUtils.loadAs(inputStream, Map.class); properties = loadedProperties != null ? new HashMap<>(loadedProperties) : new HashMap<>(); } catch (IOException | ScannerException e) { - LOG.error("Unable to parse YAML file: {}", yamlFile, e); - throw new IOException("Unable to parse snapshot properties YAML file", e); + LOG.error("Unable to parse snapshot local properties YAML file: {}", yamlFile, e); + throw new IOException("Unable to parse snapshot local properties YAML file", e); } } diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalPropertyYamlImpl.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalPropertyYamlImpl.java index 710ce03261ab..670adc69c2cc 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalPropertyYamlImpl.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalPropertyYamlImpl.java @@ -173,7 +173,6 @@ public void testRemoveProperty() throws IOException { propertyImpl.close(); - // Verify property removal is persisted try (OmSnapshotLocalPropertyYamlImpl newPropertyImpl = new OmSnapshotLocalPropertyYamlImpl(yamlFile.getAbsolutePath())) { assertFalse(newPropertyImpl.hasProperty(key)); @@ -202,16 +201,12 @@ public void testGetProperties() throws IOException { propertyImpl.setProperty(entry.getKey(), entry.getValue()); } - // Verify all properties are accessible + // Verify all properties set Map retrievedProperties = propertyImpl.getProperties(); assertEquals(testProperties.size(), retrievedProperties.size()); for (Map.Entry entry : testProperties.entrySet()) { assertEquals(entry.getValue(), retrievedProperties.get(entry.getKey())); } - - // Verify modifications to returned map don't affect internal state - retrievedProperties.put("newKey", "newValue"); - assertFalse(propertyImpl.hasProperty("newKey")); } @Test @@ -259,6 +254,6 @@ public void testInvalidYamlFile() throws IOException { IOException exception = assertThrows(IOException.class, () -> new OmSnapshotLocalPropertyYamlImpl(yamlFile.getAbsolutePath())); - assertTrue(exception.getMessage().contains("Unable to parse snapshot properties YAML file")); + assertTrue(exception.getMessage().contains("Unable to parse snapshot local properties YAML file")); } } From 6dfd198c8142569218fce64a568b21e19cb35cd9 Mon Sep 17 00:00:00 2001 From: Siyao Meng <50227127+smengcl@users.noreply.github.com> Date: Tue, 10 Jun 2025 22:47:29 -0700 Subject: [PATCH 09/20] Remove `OmSnapshotLocalProperty`, `OmSnapshotLocalPropertyImpl` and tests. --- .../ozone/om/OmSnapshotLocalProperty.java | 71 ----- .../om/OmSnapshotLocalPropertyYamlImpl.java | 178 ------------ .../TestOmSnapshotLocalPropertyYamlImpl.java | 259 ------------------ 3 files changed, 508 deletions(-) delete mode 100644 hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalProperty.java delete mode 100644 hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyYamlImpl.java delete mode 100644 hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalPropertyYamlImpl.java diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalProperty.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalProperty.java deleted file mode 100644 index 1c87d6639933..000000000000 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalProperty.java +++ /dev/null @@ -1,71 +0,0 @@ -/* - * 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 java.io.IOException; -import java.util.Map; - -/** - * Interface to manage Ozone snapshot DB local checkpoint metadata properties. - * Those properties are per-OM, e.g. isSSTFiltered flag, which differs from the ones stored in SnapshotInfo proto. - */ -public interface OmSnapshotLocalProperty extends AutoCloseable { - - /** - * Sets a property for a snapshot. - * - * @param key Property key - * @param value Property value - * @throws IOException if an I/O error occurs - */ - void setProperty(String key, String value) throws IOException; - - /** - * Gets a property value for a snapshot. - * - * @param key Property key - * @return Property value or null if not found - * @throws IOException if an I/O error occurs - */ - String getProperty(String key) throws IOException; - - /** - * Gets all properties for a snapshot. - * - * @return Map of property key-value pairs - * @throws IOException if an I/O error occurs - */ - Map getProperties() throws IOException; - - /** - * Checks if a property exists for a snapshot. - * - * @param key Property key - * @return true if the property exists, false otherwise - * @throws IOException if an I/O error occurs - */ - boolean hasProperty(String key) throws IOException; - - /** - * Removes a property from a snapshot. - * - * @param key Property key - * @throws IOException if an I/O error occurs - */ - void removeProperty(String key) throws IOException; -} diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyYamlImpl.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyYamlImpl.java deleted file mode 100644 index 5035ddb160b3..000000000000 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalPropertyYamlImpl.java +++ /dev/null @@ -1,178 +0,0 @@ -/* - * 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 java.io.File; -import java.io.IOException; -import java.io.InputStream; -import java.nio.file.Files; -import java.util.Collections; -import java.util.HashMap; -import java.util.Map; -import java.util.Objects; -import java.util.concurrent.atomic.AtomicBoolean; -import org.apache.hadoop.hdds.server.YamlUtils; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; -import org.yaml.snakeyaml.DumperOptions; -import org.yaml.snakeyaml.Yaml; -import org.yaml.snakeyaml.scanner.ScannerException; - -/** - * Implementation of {@link OmSnapshotLocalProperty} that uses a YAML file - * to store and retrieve snapshot local properties. - * Changes are only persisted when the object is closed. - */ -public class OmSnapshotLocalPropertyYamlImpl implements OmSnapshotLocalProperty, AutoCloseable { - - private static final Logger LOG = LoggerFactory.getLogger(OmSnapshotLocalPropertyYamlImpl.class); - - /** - * The path to the YAML file used to store properties. - * If this file does not exist, it will be created upon close(). - */ - private final File yamlFile; - - /** - * A map storing key-value pairs of snapshot properties. - * Read from the YAML file upon initialization. - */ - private Map properties; - - /** - * Flag indicating whether the properties have been modified - * since the last save to the YAML file. - * Used to determine if file write is needed on close. - */ - private final AtomicBoolean isDirty = new AtomicBoolean(false); - - /** - * Flag indicating whether this instance has been closed. - * Operations attempted after closing will throw an OMException. - */ - private final AtomicBoolean isClosed = new AtomicBoolean(false); - - /** - * Constructs a new OmSnapshotLocalPropertyYamlImpl. - * - * @param yamlFilePath Path to the YAML file - */ - public OmSnapshotLocalPropertyYamlImpl(String yamlFilePath) throws IOException { - this.yamlFile = new File(yamlFilePath); - loadPropertiesFromFile(); - } - - @Override - public void setProperty(String key, String value) throws IOException { - checkIfClosed(); - String oldValue = properties.get(key); - if (!Objects.equals(value, oldValue)) { - properties.put(key, value); - isDirty.set(true); - } - } - - @Override - public String getProperty(String key) throws IOException { - checkIfClosed(); - return properties.get(key); - } - - @Override - public boolean hasProperty(String key) throws IOException { - checkIfClosed(); - return properties.containsKey(key); - } - - @Override - public void removeProperty(String key) throws IOException { - checkIfClosed(); - if (properties.containsKey(key)) { - properties.remove(key); - isDirty.set(true); - } - } - - @Override - public Map getProperties() throws IOException { - checkIfClosed(); - return Collections.unmodifiableMap(properties); - } - - /** - * Saves any pending changes to the YAML file and releases resources. - * - * @throws IOException if an I/O error occurs saving the file - */ - @Override - public void close() throws IOException { - if (isClosed.compareAndSet(false, true)) { - if (isDirty.get()) { - LOG.debug("Saving changes to properties file: {}", yamlFile); - savePropertiesToFile(); - } - } - } - - /** - * Checks if the object has been closed. - * - * @throws IOException if the object has been closed - */ - private void checkIfClosed() throws IOException { - if (isClosed.get()) { - throw new IOException("OmSnapshotLocalPropertyYamlImpl has been closed"); - } - } - - /** - * Loads the properties from the YAML file. - * - * @throws IOException if an I/O error occurs, or if the YAML file is not properly formatted - */ - private void loadPropertiesFromFile() throws IOException { - if (!yamlFile.exists()) { - LOG.debug("YAML file does not exist, creating empty properties map"); - properties = new HashMap<>(); - return; - } - - try (InputStream inputStream = Files.newInputStream(yamlFile.toPath())) { - Map loadedProperties = YamlUtils.loadAs(inputStream, Map.class); - properties = loadedProperties != null ? new HashMap<>(loadedProperties) : new HashMap<>(); - } catch (IOException | ScannerException e) { - LOG.error("Unable to parse snapshot local properties YAML file: {}", yamlFile, e); - throw new IOException("Unable to parse snapshot local properties YAML file", e); - } - } - - /** - * Saves the properties to the YAML file. - * - * @throws IOException if an I/O error occurs - */ - private void savePropertiesToFile() throws IOException { - DumperOptions options = new DumperOptions(); - options.setPrettyFlow(true); - options.setDefaultFlowStyle(DumperOptions.FlowStyle.FLOW); - Yaml yaml = new Yaml(options); - - YamlUtils.dump(yaml, properties, yamlFile, LOG); - isDirty.set(false); - } -} diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalPropertyYamlImpl.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalPropertyYamlImpl.java deleted file mode 100644 index 670adc69c2cc..000000000000 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalPropertyYamlImpl.java +++ /dev/null @@ -1,259 +0,0 @@ -/* - * 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 static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertNull; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; - -import java.io.File; -import java.io.IOException; -import java.nio.file.Files; -import java.nio.file.Path; -import java.util.HashMap; -import java.util.Map; -import org.apache.hadoop.hdds.server.YamlUtils; -import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.io.TempDir; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; -import org.yaml.snakeyaml.DumperOptions; -import org.yaml.snakeyaml.Yaml; - -/** - * Tests for {@link OmSnapshotLocalPropertyYamlImpl}. - */ -public class TestOmSnapshotLocalPropertyYamlImpl { - - private static final Logger LOG = LoggerFactory.getLogger(TestOmSnapshotLocalPropertyYamlImpl.class); - - @TempDir - private Path tempDir; - - private File yamlFile; - private OmSnapshotLocalPropertyYamlImpl propertyImpl; - - @BeforeEach - public void setUp() throws IOException { - yamlFile = tempDir.resolve("test-properties.yaml").toFile(); - propertyImpl = new OmSnapshotLocalPropertyYamlImpl(yamlFile.getAbsolutePath()); - } - - @AfterEach - public void tearDown() throws IOException { - if (propertyImpl != null) { - propertyImpl.close(); - } - - if (yamlFile.exists()) { - yamlFile.delete(); - } - } - - @Test - public void testInitWithNonExistentFile() throws IOException { - // Verify that the implementation carries an empty properties map when the YAML file doesn't exist - Map properties = propertyImpl.getProperties(); - assertNotNull(properties); - assertTrue(properties.isEmpty()); - - // Now set a property and close to create the file - String key = "testKey"; - String value = "testValue"; - propertyImpl.setProperty(key, value); - - // File should not be created until close is called - assertFalse(yamlFile.exists(), "YAML file should not exist before close"); - - propertyImpl.close(); - assertTrue(yamlFile.exists(), "YAML file should be created on close"); - } - - @Test - public void testSetAndGetProperty() throws IOException { - // Set a property - String key = "testKey"; - String value = "testValue"; - propertyImpl.setProperty(key, value); - - // Verify the property was set in memory - assertEquals(value, propertyImpl.getProperty(key)); - assertTrue(propertyImpl.hasProperty(key)); - - // Verify the file is created with correct content after close - propertyImpl.close(); - assertTrue(yamlFile.exists()); - - // Read back for verification - try (OmSnapshotLocalPropertyYamlImpl newPropertyImpl = - new OmSnapshotLocalPropertyYamlImpl(yamlFile.getAbsolutePath())) { - assertEquals(value, newPropertyImpl.getProperty(key)); - } - } - - @Test - public void testSetPropertyNoWriteUponCloseWhenNotDirty() throws IOException { - // Setting the same property with the same value should not mark as dirty - String key = "testKey"; - String value = "testValue"; - - propertyImpl.setProperty(key, value); - propertyImpl.close(); - - // close() should create the file if it doesn't exist in this case - assertTrue(yamlFile.exists()); - - // Store the last modification time - long lastModified = yamlFile.lastModified(); - - propertyImpl = new OmSnapshotLocalPropertyYamlImpl(yamlFile.getAbsolutePath()); - // Same key-value, should not be dirty - propertyImpl.setProperty(key, value); - propertyImpl.close(); - - // File modification time should not have changed since nothing changed - assertEquals(lastModified, yamlFile.lastModified(), - "File should not have been modified since properties didn't change"); - } - - @Test - public void testUpdateProperty() throws IOException { - // Set and then update a property - String key = "testKey"; - String originalValue = "originalValue"; - String updatedValue = "updatedValue"; - - propertyImpl.setProperty(key, originalValue); - assertEquals(originalValue, propertyImpl.getProperty(key)); - - propertyImpl.setProperty(key, updatedValue); - assertEquals(updatedValue, propertyImpl.getProperty(key)); - - propertyImpl.close(); - - // Verify updated value is persisted - try (OmSnapshotLocalPropertyYamlImpl newPropertyImpl = - new OmSnapshotLocalPropertyYamlImpl(yamlFile.getAbsolutePath())) { - assertEquals(updatedValue, newPropertyImpl.getProperty(key)); - } - } - - @Test - public void testRemoveProperty() throws IOException { - // Set and then remove a property - String key = "testKey"; - String value = "testValue"; - - propertyImpl.setProperty(key, value); - assertTrue(propertyImpl.hasProperty(key)); - - propertyImpl.removeProperty(key); - assertFalse(propertyImpl.hasProperty(key)); - assertNull(propertyImpl.getProperty(key)); - - propertyImpl.close(); - - try (OmSnapshotLocalPropertyYamlImpl newPropertyImpl = - new OmSnapshotLocalPropertyYamlImpl(yamlFile.getAbsolutePath())) { - assertFalse(newPropertyImpl.hasProperty(key)); - } - } - - @Test - public void testRemoveNonExistentProperty() throws IOException { - // Removing a non-existent property should not cause errors - propertyImpl.removeProperty("nonExistentKey"); - - // Should not mark as dirty, so file shouldn't be created on close - propertyImpl.close(); - assertFalse(yamlFile.exists()); - } - - @Test - public void testGetProperties() throws IOException { - // Set multiple properties - Map testProperties = new HashMap<>(); - testProperties.put("key1", "value1"); - testProperties.put("key2", "value2"); - testProperties.put("key3", "value3"); - - for (Map.Entry entry : testProperties.entrySet()) { - propertyImpl.setProperty(entry.getKey(), entry.getValue()); - } - - // Verify all properties set - Map retrievedProperties = propertyImpl.getProperties(); - assertEquals(testProperties.size(), retrievedProperties.size()); - for (Map.Entry entry : testProperties.entrySet()) { - assertEquals(entry.getValue(), retrievedProperties.get(entry.getKey())); - } - } - - @Test - public void testLoadFromExistingFile() throws IOException { - // Create a file with predefined properties - Map initialProperties = new HashMap<>(); - initialProperties.put("existing1", "value1"); - initialProperties.put("existing2", "value2"); - - DumperOptions options = new DumperOptions(); - options.setPrettyFlow(true); - options.setDefaultFlowStyle(DumperOptions.FlowStyle.FLOW); - Yaml yaml = new Yaml(options); - - YamlUtils.dump(yaml, initialProperties, yamlFile, LOG); - - // Instantiate with existing file - try (OmSnapshotLocalPropertyYamlImpl loadedPropertyImpl = - new OmSnapshotLocalPropertyYamlImpl(yamlFile.getAbsolutePath())) { - // Verify properties are loaded correctly - assertEquals("value1", loadedPropertyImpl.getProperty("existing1")); - assertEquals("value2", loadedPropertyImpl.getProperty("existing2")); - assertEquals(2, loadedPropertyImpl.getProperties().size()); - } - } - - @Test - public void testOperationsAfterClose() throws IOException { - propertyImpl.close(); - - // All operations should throw IOException after close - assertThrows(IOException.class, () -> propertyImpl.getProperty("anyKey")); - assertThrows(IOException.class, () -> propertyImpl.setProperty("anyKey", "anyValue")); - assertThrows(IOException.class, () -> propertyImpl.hasProperty("anyKey")); - assertThrows(IOException.class, () -> propertyImpl.removeProperty("anyKey")); - assertThrows(IOException.class, () -> propertyImpl.getProperties()); - } - - @Test - public void testInvalidYamlFile() throws IOException { - // Create file with invalid YAML content - Files.write(yamlFile.toPath(), "invalid: yaml: content: - not properly formatted".getBytes()); - - // Should throw IOException with appropriate message - IOException exception = assertThrows(IOException.class, - () -> new OmSnapshotLocalPropertyYamlImpl(yamlFile.getAbsolutePath())); - - assertTrue(exception.getMessage().contains("Unable to parse snapshot local properties YAML file")); - } -} From 545ceb5bfb733ed4393f58ab3375e2c46b24a528 Mon Sep 17 00:00:00 2001 From: Siyao Meng <50227127+smengcl@users.noreply.github.com> Date: Tue, 10 Jun 2025 23:45:06 -0700 Subject: [PATCH 10/20] Add `OmSnapshotLocalData` and its YAML impl; add UTs; removed added logic in `SstFilteringService` --- .../org/apache/hadoop/ozone/OzoneConsts.java | 7 + .../hadoop/ozone/om/OmSnapshotLocalData.java | 243 +++++++++++++++ .../ozone/om/OmSnapshotLocalDataYaml.java | 282 ++++++++++++++++++ .../hadoop/ozone/om/SstFilteringService.java | 22 +- .../ozone/om/TestOmSnapshotLocalDataYaml.java | 239 +++++++++++++++ 5 files changed, 773 insertions(+), 20 deletions(-) create mode 100644 hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalData.java create mode 100644 hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java create mode 100644 hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java index 6bee926336b8..140924d5cfa9 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java @@ -202,6 +202,13 @@ public final class OzoneConsts { public static final String SCM_CONTEXT_ATTRIBUTE = "ozone.scm"; + // YAML field constants for OmSnapshotLocalData .snapshot files + public static final String IS_SST_FILTERED = "isSSTFiltered"; + public static final String UNCOMPACTED_SST_FILE_LIST = "uncompactedSSTFileList"; + public static final String LAST_COMPACTION_TIME = "lastCompactionTime"; + public static final String NEEDS_COMPACTION = "needsCompaction"; + public static final String COMPACTED_SST_FILE_LIST = "compactedSSTFileList"; + // YAML fields for .container files public static final String CONTAINER_ID = "containerID"; public static final String CONTAINER_TYPE = "containerType"; diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalData.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalData.java new file mode 100644 index 000000000000..06433b290c84 --- /dev/null +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalData.java @@ -0,0 +1,243 @@ +/* + * 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 com.google.common.collect.Lists; +import com.google.common.collect.Maps; +import java.io.IOException; +import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; +import java.sql.Timestamp; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import org.apache.commons.codec.digest.DigestUtils; +import org.apache.hadoop.ozone.OzoneConsts; +import org.yaml.snakeyaml.Yaml; + +/** + * OmSnapshotLocalData is the in-memory representation of snapshot local metadata. + * Inspired by {@link org.apache.hadoop.ozone.container.common.impl.ContainerData} + */ +public class OmSnapshotLocalData { + + // Whether SST is filtered + private boolean isSSTFiltered; + + // Map of Table to uncompacted SST file list on snapshot create + private Map> uncompactedSSTFileList; + + // Timestamp of the last compaction + private Timestamp lastCompactionTime; + + // Whether the snapshot needs compaction + private boolean needsCompaction; + + // Map of version to compacted SST file list + // Map> + private Map>> compactedSSTFileList; + + // Checksum of the YAML representation + private String checksum; + + // Common Fields that need to be stored in the yaml file + public static final List YAML_FIELDS = + Collections.unmodifiableList(Lists.newArrayList( + OzoneConsts.CHECKSUM, + OzoneConsts.IS_SST_FILTERED, + OzoneConsts.UNCOMPACTED_SST_FILE_LIST, + OzoneConsts.LAST_COMPACTION_TIME, + OzoneConsts.NEEDS_COMPACTION, + OzoneConsts.COMPACTED_SST_FILE_LIST)); + + public static final Charset CHARSET_ENCODING = StandardCharsets.UTF_8; + private static final String DUMMY_CHECKSUM = new String(new byte[64], CHARSET_ENCODING); + + /** + * Creates a OmSnapshotLocalData object with default values. + */ + public OmSnapshotLocalData() { + this.isSSTFiltered = false; + this.uncompactedSSTFileList = new HashMap<>(); + this.lastCompactionTime = new Timestamp(0); + this.needsCompaction = false; + this.compactedSSTFileList = new HashMap<>(); + setChecksumTo0ByteArray(); + } + + /** + * Returns whether SST is filtered. + * @return true if SST is filtered, false otherwise + */ + public boolean isSstFiltered() { + return isSSTFiltered; + } + + /** + * Sets whether SST is filtered. + * @param sstFiltered + */ + public void setSstFiltered(boolean sstFiltered) { + this.isSSTFiltered = sstFiltered; + } + + /** + * Returns the uncompacted SST file list. + * @return Map of Table to uncompacted SST file list + */ + public Map> getUncompactedSSTFileList() { + return Collections.unmodifiableMap(this.uncompactedSSTFileList); + } + + /** + * Sets the uncompacted SST file list. + * @param uncompactedSSTFileList Map of Table to uncompacted SST file list + */ + public void setUncompactedSSTFileList( + Map> uncompactedSSTFileList) { + this.uncompactedSSTFileList.clear(); + this.uncompactedSSTFileList.putAll(uncompactedSSTFileList); + } + + /** + * Adds an entry to the uncompacted SST file list. + * @param table Table name + * @param sstFile SST file name + */ + public void addUncompactedSSTFile(String table, String sstFile) { + this.uncompactedSSTFileList.computeIfAbsent(table, k -> Lists.newArrayList()) + .add(sstFile); + } + + /** + * Returns the last compaction time. + * @return Timestamp of the last compaction + */ + public Timestamp getLastCompactionTime() { + return lastCompactionTime; + } + + /** + * Sets the last compaction time. + * @param lastCompactionTime Timestamp of the last compaction + */ + public void setLastCompactionTime(Timestamp lastCompactionTime) { + this.lastCompactionTime = lastCompactionTime; + } + + /** + * Returns whether the snapshot needs compaction. + * @return true if the snapshot needs compaction, false otherwise + */ + public boolean isNeedsCompaction() { + return needsCompaction; + } + + /** + * Sets whether the snapshot needs compaction. + * @param needsCompaction true if the snapshot needs compaction, false otherwise + */ + public void setNeedsCompaction(boolean needsCompaction) { + this.needsCompaction = needsCompaction; + } + + /** + * Returns the compacted SST file list. + * @return Map of version to compacted SST file list + */ + public Map>> getCompactedSSTFileList() { + return Collections.unmodifiableMap(this.compactedSSTFileList); + } + + /** + * Sets the compacted SST file list. + * @param compactedSSTFileList Map of version to compacted SST file list + */ + public void setCompactedSSTFileList( + Map>> compactedSSTFileList) { + this.compactedSSTFileList.clear(); + this.compactedSSTFileList.putAll(compactedSSTFileList); + } + + /** + * Adds an entry to the compacted SST file list. + * @param version Version number + * @param table Table name + * @param sstFile SST file name + */ + public void addCompactedSSTFile(Integer version, String table, String sstFile) { + this.compactedSSTFileList.computeIfAbsent(version, k -> Maps.newHashMap()) + .computeIfAbsent(table, k -> Lists.newArrayList()) + .add(sstFile); + } + + /** + * Returns the checksum of the YAML representation. + * @return checksum + */ + public String getChecksum() { + return checksum; + } + + /** + * Sets the checksum of the YAML representation. + * @param checksum checksum + */ + public void setChecksum(String checksum) { + this.checksum = checksum; + } + + /** + * Sets the checksum to a 0 byte array. + */ + public void setChecksumTo0ByteArray() { + this.checksum = DUMMY_CHECKSUM; + } + + /** + * Compute and set checksum for the snapshot data. + * @param yaml Yaml instance for serialization + * @throws IOException if checksum computation fails + */ + public void computeAndSetChecksum(Yaml yaml) throws IOException { + // Set checksum to dummy value - 0 byte array, to calculate the checksum + // of rest of the data. + setChecksumTo0ByteArray(); + + // Dump yaml data into a string to compute its checksum + String snapshotDataYamlStr = yaml.dump(this); + + this.checksum = getChecksum(snapshotDataYamlStr); + } + + /** + * Computes SHA-256 hash for a given string. + * + * @param data String data for which checksum needs to be calculated + * @return SHA-256 checksum as hex string + * @throws IOException If checksum calculation fails + */ + private static String getChecksum(String data) throws IOException { + try { + return DigestUtils.sha256Hex(data.getBytes(StandardCharsets.UTF_8)); + } catch (Exception ex) { + throw new IOException("Unable to calculate checksum", ex); + } + } +} diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java new file mode 100644 index 000000000000..c3b25f3da162 --- /dev/null +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java @@ -0,0 +1,282 @@ +/* + * 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 com.google.common.base.Preconditions; +import java.io.ByteArrayInputStream; +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; +import java.sql.Timestamp; +import java.time.Instant; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.TreeSet; +import org.apache.hadoop.hdds.server.YamlUtils; +import org.apache.hadoop.ozone.OzoneConsts; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.yaml.snakeyaml.DumperOptions; +import org.yaml.snakeyaml.LoaderOptions; +import org.yaml.snakeyaml.Yaml; +import org.yaml.snakeyaml.constructor.AbstractConstruct; +import org.yaml.snakeyaml.constructor.SafeConstructor; +import org.yaml.snakeyaml.error.YAMLException; +import org.yaml.snakeyaml.introspector.BeanAccess; +import org.yaml.snakeyaml.introspector.Property; +import org.yaml.snakeyaml.introspector.PropertyUtils; +import org.yaml.snakeyaml.nodes.MappingNode; +import org.yaml.snakeyaml.nodes.Node; +import org.yaml.snakeyaml.nodes.NodeTuple; +import org.yaml.snakeyaml.nodes.Tag; +import org.yaml.snakeyaml.representer.Representer; + +/** + * Class for creating and reading snapshot data YAML files. + * Checksum of the YAML fields are computed and stored in the YAML file. + * Inspired by {@link org.apache.hadoop.ozone.container.common.impl.ContainerDataYaml} + */ +public final class OmSnapshotLocalDataYaml { + + private static final Logger LOG = + LoggerFactory.getLogger(OmSnapshotLocalDataYaml.class); + + public static final Tag SNAPSHOT_YAML_TAG = new Tag("OmSnapshotLocalData"); + + private OmSnapshotLocalDataYaml() { + + } + + /** + * Creates a snapshot data file in YAML format. + */ + public static void createSnapshotFile(OmSnapshotLocalData snapshotData, + File snapshotFile) throws IOException { + // Create Yaml + final Yaml yaml = getYamlForSnapshotData(); + // Compute Checksum and update SnapshotData + snapshotData.computeAndSetChecksum(yaml); + + // Write the SnapshotData with checksum to Yaml file. + YamlUtils.dump(yaml, snapshotData, snapshotFile, LOG); + } + + /** + * Read the YAML file, and return snapshotData. + * + * @throws IOException + */ + public static OmSnapshotLocalData readSnapshotFile(File snapshotFile) + throws IOException { + Preconditions.checkNotNull(snapshotFile, "snapshotFile cannot be null"); + try (InputStream inputFileStream = Files.newInputStream( + snapshotFile.toPath())) { + return readSnapshot(inputFileStream); + } + } + + /** + * Read the YAML file content, and return snapshotData. + * + * @throws IOException + */ + public static OmSnapshotLocalData readSnapshot(byte[] snapshotFileContent) + throws IOException { + return readSnapshot( + new ByteArrayInputStream(snapshotFileContent)); + } + + /** + * Read the YAML content, and return snapshotData. + * + * @throws IOException + */ + public static OmSnapshotLocalData readSnapshot(InputStream input) + throws IOException { + OmSnapshotLocalData snapshotData; + PropertyUtils propertyUtils = new PropertyUtils(); + propertyUtils.setBeanAccess(BeanAccess.FIELD); + propertyUtils.setAllowReadOnlyProperties(true); + + Representer representer = new SnapshotDataRepresenter( + OmSnapshotLocalData.YAML_FIELDS); + representer.setPropertyUtils(propertyUtils); + + SafeConstructor snapshotDataConstructor = new SnapshotDataConstructor(); + + Yaml yaml = new Yaml(snapshotDataConstructor, representer); + yaml.setBeanAccess(BeanAccess.FIELD); + + try { + snapshotData = yaml.load(input); + } catch (YAMLException ex) { + // Unchecked exception. Convert to IOException + throw new IOException(ex); + } + + if (snapshotData == null) { + // If Yaml#load returned null, then the file is empty. This is valid yaml + // but considered an error in this case since we have lost data about + // the snapshot. + throw new IOException("Failed to load snapshot file. File is empty."); + } + + return snapshotData; + } + + /** + * Returns a Yaml representation of the snapshot properties. + * + * @return Yaml representation of snapshot properties + */ + public static Yaml getYamlForSnapshotData() { + PropertyUtils propertyUtils = new PropertyUtils(); + propertyUtils.setBeanAccess(BeanAccess.FIELD); + propertyUtils.setAllowReadOnlyProperties(true); + + Representer representer = new SnapshotDataRepresenter( + OmSnapshotLocalData.YAML_FIELDS); + representer.setPropertyUtils(propertyUtils); + representer.addClassTag(OmSnapshotLocalData.class, SNAPSHOT_YAML_TAG); + + SafeConstructor snapshotDataConstructor = new SnapshotDataConstructor(); + + return new Yaml(snapshotDataConstructor, representer); + } + + /** + * Representer class to define which fields need to be stored in yaml file. + */ + private static class SnapshotDataRepresenter extends Representer { + + private List yamlFields; + + SnapshotDataRepresenter(List yamlFields) { + super(new DumperOptions()); + this.yamlFields = yamlFields; + + // Add custom representer for Timestamp to output epoch milliseconds + representers.put(Timestamp.class, data -> { + Timestamp timestamp = (Timestamp) data; + return represent(timestamp.getTime()); + }); + } + + @Override + protected Set getProperties(Class type) { + Set set = super.getProperties(type); + Set filtered = new TreeSet(); + + if (type.equals(OmSnapshotLocalData.class)) { + // filter properties + for (Property prop : set) { + String name = prop.getName(); + if (yamlFields.contains(name)) { + filtered.add(prop); + } + } + } + return filtered; + } + + /** + * Omit properties with null value. + */ + @Override + protected NodeTuple representJavaBeanProperty( + Object bean, Property property, Object value, Tag tag) { + return value == null + ? null + : super.representJavaBeanProperty(bean, property, value, tag); + } + } + + /** + * Constructor class for OmSnapshotLocalData, which will be used by Yaml. + */ + private static class SnapshotDataConstructor extends SafeConstructor { + SnapshotDataConstructor() { + super(new LoaderOptions()); + //Adding our own specific constructors for tags. + this.yamlConstructors.put( + SNAPSHOT_YAML_TAG, new ConstructSnapshotData()); + } + + private final class ConstructSnapshotData extends AbstractConstruct { + @SuppressWarnings("unchecked") + @Override + public Object construct(Node node) { + MappingNode mnode = (MappingNode) node; + Map nodes = constructMapping(mnode); + + OmSnapshotLocalData snapshotData = new OmSnapshotLocalData(); + + // Set fields from parsed YAML + snapshotData.setSstFiltered((Boolean) nodes.getOrDefault( + OzoneConsts.IS_SST_FILTERED, false)); + + Map> uncompactedSSTFileList = + (Map>) nodes.get( + OzoneConsts.UNCOMPACTED_SST_FILE_LIST); + if (uncompactedSSTFileList != null) { + snapshotData.setUncompactedSSTFileList(uncompactedSSTFileList); + } + + Object lastCompactionTimeObj = nodes.get( + OzoneConsts.LAST_COMPACTION_TIME); + if (lastCompactionTimeObj != null) { + if (lastCompactionTimeObj instanceof Long) { + snapshotData.setLastCompactionTime( + new Timestamp((Long) lastCompactionTimeObj)); + } else if (lastCompactionTimeObj instanceof Timestamp) { + snapshotData.setLastCompactionTime( + (Timestamp) lastCompactionTimeObj); + } else if (lastCompactionTimeObj instanceof String) { + // Handle timestamp as ISO-8601 string (common in YAML) + try { + Instant instant = Instant.parse((String) lastCompactionTimeObj); + snapshotData.setLastCompactionTime(new Timestamp(instant.toEpochMilli())); + } catch (Exception e) { + // If parsing fails, use default timestamp + snapshotData.setLastCompactionTime(new Timestamp(0)); + } + } + } + + snapshotData.setNeedsCompaction((Boolean) nodes.getOrDefault( + OzoneConsts.NEEDS_COMPACTION, false)); + + Map>> compactedSSTFileList = + (Map>>) nodes.get( + OzoneConsts.COMPACTED_SST_FILE_LIST); + if (compactedSSTFileList != null) { + snapshotData.setCompactedSSTFileList(compactedSSTFileList); + } + + String checksum = (String) nodes.get(OzoneConsts.CHECKSUM); + if (checksum != null) { + snapshotData.setChecksum(checksum); + } + + return snapshotData; + } + } + } +} diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java index 89be2253422d..a5c34d65da50 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java @@ -87,16 +87,7 @@ public class SstFilteringService extends BackgroundService private final BootstrapStateHandler.Lock lock = new BootstrapStateHandler.Lock(); public static boolean isSstFiltered(OzoneConfiguration ozoneConfiguration, SnapshotInfo snapshotInfo) { - // First try to read the flag from YAML file - String yamlPath = OmSnapshotManager.getSnapshotLocalPropertyYamlPath(ozoneConfiguration, snapshotInfo); - - try (OmSnapshotLocalProperty localProperties = new OmSnapshotLocalPropertyYamlImpl(yamlPath)) { - String sstFilteredProperty = localProperties.getProperty(SST_FILTERED_YAML_KEY); - return Boolean.parseBoolean(sstFilteredProperty); - } catch (Exception e) { - // If we can't read the YAML file, fall back to the existing checks - LOG.debug("Failed to read snapshot local properties from YAML file: {}", yamlPath, e); - } + // TODO: First try to read the YAML file // Fall back to existing checks Path sstFilteredFile = Paths.get( @@ -156,16 +147,7 @@ private void markSSTFilteredFlagForSnapshot(SnapshotInfo snapshotInfo) throws IO try { // Mark the snapshot as filtered by writing to YAML property file if (Files.exists(Paths.get(snapshotDir))) { - String yamlPath = OmSnapshotManager.getSnapshotLocalPropertyYamlPath( - ozoneManager.getConfiguration(), snapshotInfo); - try (OmSnapshotLocalProperty localProperties = new OmSnapshotLocalPropertyYamlImpl(yamlPath)) { - localProperties.setProperty(SST_FILTERED_YAML_KEY, "true"); - } catch (Exception e) { - LOG.error("Failed to set SST filtered local property for snapshot: {}", snapshotInfo.getName(), e); - } - - // For backward compatibility, still create the touch file (e.g. when upgraded but not finalized yet) - // TODO: When upgrade is finalized, this can be skipped + // TODO: Write to YAML Files.write(Paths.get(snapshotDir, SST_FILTERED_FILE), SST_FILTERED_FILE_CONTENT); } } finally { diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java new file mode 100644 index 000000000000..72f6841f6295 --- /dev/null +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java @@ -0,0 +1,239 @@ +/* + * 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 static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.File; +import java.io.IOException; +import java.nio.charset.Charset; +import java.sql.Timestamp; +import java.time.Instant; +import java.util.List; +import java.util.Map; +import org.apache.commons.io.FileUtils; +import org.apache.hadoop.fs.FileSystemTestHelper; +import org.apache.hadoop.fs.FileUtil; +import org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.ozone.OzoneConsts; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.yaml.snakeyaml.Yaml; + +/** + * This class tests creating and reading snapshot data YAML files. + */ +public class TestOmSnapshotLocalDataYaml { + + private static String testRoot = new FileSystemTestHelper().getTestRootDir(); + + private static final Instant NOW = Instant.now(); + private OzoneConfiguration conf = new OzoneConfiguration(); + + @BeforeEach + public void setUp() { + assertTrue(new File(testRoot).mkdirs()); + } + + @AfterEach + public void cleanup() { + FileUtil.fullyDelete(new File(testRoot)); + } + + /** + * Creates a snapshot data YAML file. + */ + private File createSnapshotDataFile(String snapshotName) throws IOException { + String yamlFilePath = snapshotName + ".yaml"; + + OmSnapshotLocalData snapshotData = new OmSnapshotLocalData(); + snapshotData.setSstFiltered(true); + + // Add some uncompacted SST files + snapshotData.addUncompactedSSTFile("table1", "sst1"); + snapshotData.addUncompactedSSTFile("table1", "sst2"); + snapshotData.addUncompactedSSTFile("table2", "sst3"); + + // Set last compaction time + snapshotData.setLastCompactionTime(new Timestamp(NOW.toEpochMilli())); + + // Set needs compaction flag + snapshotData.setNeedsCompaction(true); + + // Add some compacted SST files + snapshotData.addCompactedSSTFile(1, "table1", "compacted-sst1"); + snapshotData.addCompactedSSTFile(1, "table2", "compacted-sst2"); + snapshotData.addCompactedSSTFile(2, "table1", "compacted-sst3"); + + File yamlFile = new File(testRoot, yamlFilePath); + + // Create YAML file with SnapshotData + OmSnapshotLocalDataYaml.createSnapshotFile(snapshotData, yamlFile); + + // Check YAML file exists + assertTrue(yamlFile.exists()); + + return yamlFile; + } + + @Test + public void testCreateSnapshotDataFile() throws IOException { + File yamlFile = createSnapshotDataFile("snapshot1"); + + // Read from YAML file, and verify data + OmSnapshotLocalData snapshotData = + OmSnapshotLocalDataYaml.readSnapshotFile(yamlFile); + + assertTrue(snapshotData.isSstFiltered()); + + Map> uncompactedFiles = snapshotData.getUncompactedSSTFileList(); + assertEquals(2, uncompactedFiles.size()); + assertEquals(2, uncompactedFiles.get("table1").size()); + assertEquals(1, uncompactedFiles.get("table2").size()); + assertTrue(uncompactedFiles.get("table1").contains("sst1")); + assertTrue(uncompactedFiles.get("table1").contains("sst2")); + assertTrue(uncompactedFiles.get("table2").contains("sst3")); + + assertEquals(NOW.toEpochMilli(), snapshotData.getLastCompactionTime().getTime()); + assertTrue(snapshotData.isNeedsCompaction()); + + Map>> compactedFiles = snapshotData.getCompactedSSTFileList(); + assertEquals(2, compactedFiles.size()); + assertTrue(compactedFiles.containsKey(1)); + assertTrue(compactedFiles.containsKey(2)); + assertEquals(2, compactedFiles.get(1).size()); + assertEquals(1, compactedFiles.get(2).size()); + assertTrue(compactedFiles.get(1).get("table1").contains("compacted-sst1")); + assertTrue(compactedFiles.get(1).get("table2").contains("compacted-sst2")); + assertTrue(compactedFiles.get(2).get("table1").contains("compacted-sst3")); + } + + @Test + public void testUpdateSnapshotDataFile() throws IOException { + File yamlFile = createSnapshotDataFile("snapshot2"); + + // Read from YAML file + OmSnapshotLocalData snapshotData = + OmSnapshotLocalDataYaml.readSnapshotFile(yamlFile); + + // Update snapshot data + snapshotData.setSstFiltered(false); + snapshotData.setNeedsCompaction(false); + snapshotData.addUncompactedSSTFile("table3", "sst4"); + snapshotData.addCompactedSSTFile(3, "table3", "compacted-sst4"); + + // Write updated data back to file + OmSnapshotLocalDataYaml.createSnapshotFile(snapshotData, yamlFile); + + // Read back the updated data + snapshotData = OmSnapshotLocalDataYaml.readSnapshotFile(yamlFile); + + // Verify updated data + assertThat(snapshotData.isSstFiltered()).isFalse(); + assertThat(snapshotData.isNeedsCompaction()).isFalse(); + + Map> uncompactedFiles = snapshotData.getUncompactedSSTFileList(); + assertEquals(3, uncompactedFiles.size()); + assertTrue(uncompactedFiles.containsKey("table3")); + assertTrue(uncompactedFiles.get("table3").contains("sst4")); + + Map>> compactedFiles = snapshotData.getCompactedSSTFileList(); + assertEquals(3, compactedFiles.size()); + assertTrue(compactedFiles.containsKey(3)); + assertTrue(compactedFiles.get(3).containsKey("table3")); + assertTrue(compactedFiles.get(3).get("table3").contains("compacted-sst4")); + } + + @Test + public void testEmptyFile() throws IOException { + File emptyFile = new File(testRoot, "empty.yaml"); + assertTrue(emptyFile.createNewFile()); + + IOException ex = assertThrows(IOException.class, () -> + OmSnapshotLocalDataYaml.readSnapshotFile(emptyFile)); + + assertThat(ex).hasMessageContaining("Failed to load snapshot file. File is empty."); + } + + @Test + public void testChecksum() throws IOException { + File yamlFile = createSnapshotDataFile("snapshot3"); + + // Read from YAML file + OmSnapshotLocalData snapshotData = + OmSnapshotLocalDataYaml.readSnapshotFile(yamlFile); + + // Get the original checksum + String originalChecksum = snapshotData.getChecksum(); + + // Verify the checksum is not null or empty + assertThat(originalChecksum).isNotNull().isNotEmpty(); + + // Save the current timestamp value + Timestamp originalTimestamp = snapshotData.getLastCompactionTime(); + + // Recompute the checksum + Yaml yaml = OmSnapshotLocalDataYaml.getYamlForSnapshotData(); + snapshotData.computeAndSetChecksum(yaml); + + // Get the newly computed checksum + String recomputedChecksum = snapshotData.getChecksum(); + + // Create a new snapshot with the same data to verify checksum calculation + OmSnapshotLocalData newSnapshot = new OmSnapshotLocalData(); + newSnapshot.setSstFiltered(snapshotData.isSstFiltered()); + newSnapshot.setUncompactedSSTFileList(snapshotData.getUncompactedSSTFileList()); + newSnapshot.setLastCompactionTime(originalTimestamp); + newSnapshot.setNeedsCompaction(snapshotData.isNeedsCompaction()); + newSnapshot.setCompactedSSTFileList(snapshotData.getCompactedSSTFileList()); + + // Compute checksum for the new snapshot + newSnapshot.computeAndSetChecksum(yaml); + + // Verify the checksum of the new snapshot matches the recomputed one + assertEquals(recomputedChecksum, newSnapshot.getChecksum()); + + // Modify data and verify checksum changes + newSnapshot.addUncompactedSSTFile("table4", "sst5"); + newSnapshot.computeAndSetChecksum(yaml); + + assertThat(newSnapshot.getChecksum()) + .isNotNull() + .isNotEmpty() + .isNotEqualTo(recomputedChecksum); + } + + @Test + public void testYamlContainsAllFields() throws IOException { + File yamlFile = createSnapshotDataFile("snapshot4"); + + String content = FileUtils.readFileToString(yamlFile, Charset.defaultCharset()); + + // Verify the YAML content contains all expected fields + assertThat(content).contains(OzoneConsts.IS_SST_FILTERED); + assertThat(content).contains(OzoneConsts.UNCOMPACTED_SST_FILE_LIST); + assertThat(content).contains(OzoneConsts.LAST_COMPACTION_TIME); + assertThat(content).contains(OzoneConsts.NEEDS_COMPACTION); + assertThat(content).contains(OzoneConsts.COMPACTED_SST_FILE_LIST); + assertThat(content).contains(OzoneConsts.CHECKSUM); + } +} From 164652daea5cc52f322449ba3db0b642fba2ba10 Mon Sep 17 00:00:00 2001 From: Siyao Meng <50227127+smengcl@users.noreply.github.com> Date: Thu, 12 Jun 2025 00:58:56 -0700 Subject: [PATCH 11/20] Polish --- .../org/apache/hadoop/ozone/OzoneConsts.java | 2 +- .../hadoop/ozone/om/OmSnapshotLocalData.java | 67 ++++++-- .../ozone/om/OmSnapshotLocalDataYaml.java | 143 +++++++++--------- .../ozone/om/TestOmSnapshotLocalDataYaml.java | 80 +++------- 4 files changed, 146 insertions(+), 146 deletions(-) diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java index 140924d5cfa9..d383a0686d7f 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java @@ -202,7 +202,7 @@ public final class OzoneConsts { public static final String SCM_CONTEXT_ATTRIBUTE = "ozone.scm"; - // YAML field constants for OmSnapshotLocalData .snapshot files + // YAML field constants for OmSnapshotLocalData .yaml files public static final String IS_SST_FILTERED = "isSSTFiltered"; public static final String UNCOMPACTED_SST_FILE_LIST = "uncompactedSSTFileList"; public static final String LAST_COMPACTION_TIME = "lastCompactionTime"; diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalData.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalData.java index 06433b290c84..c37c0abf562e 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalData.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalData.java @@ -22,7 +22,6 @@ import java.io.IOException; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; -import java.sql.Timestamp; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -37,14 +36,17 @@ */ public class OmSnapshotLocalData { + // Checksum of the YAML representation + private String checksum; + // Whether SST is filtered private boolean isSSTFiltered; // Map of Table to uncompacted SST file list on snapshot create private Map> uncompactedSSTFileList; - // Timestamp of the last compaction - private Timestamp lastCompactionTime; + // Time of last compaction, in epoch milliseconds + private long lastCompactionTime; // Whether the snapshot needs compaction private boolean needsCompaction; @@ -53,9 +55,6 @@ public class OmSnapshotLocalData { // Map> private Map>> compactedSSTFileList; - // Checksum of the YAML representation - private String checksum; - // Common Fields that need to be stored in the yaml file public static final List YAML_FIELDS = Collections.unmodifiableList(Lists.newArrayList( @@ -75,22 +74,59 @@ public class OmSnapshotLocalData { public OmSnapshotLocalData() { this.isSSTFiltered = false; this.uncompactedSSTFileList = new HashMap<>(); - this.lastCompactionTime = new Timestamp(0); + this.lastCompactionTime = 0L; this.needsCompaction = false; this.compactedSSTFileList = new HashMap<>(); setChecksumTo0ByteArray(); } /** - * Returns whether SST is filtered. + * Copy constructor to create a deep copy of OmSnapshotLocalData object. + * @param source The source OmSnapshotLocalData to copy from + */ + public OmSnapshotLocalData(OmSnapshotLocalData source) { + // Copy primitive fields directly + this.isSSTFiltered = source.isSSTFiltered; + this.lastCompactionTime = source.lastCompactionTime; + this.needsCompaction = source.needsCompaction; + this.checksum = source.checksum; + + // Deep copy for uncompactedSSTFileList + this.uncompactedSSTFileList = new HashMap<>(); + for (Map.Entry> entry : + source.uncompactedSSTFileList.entrySet()) { + this.uncompactedSSTFileList.put( + entry.getKey(), + Lists.newArrayList(entry.getValue())); + } + + // Deep copy for compactedSSTFileList + this.compactedSSTFileList = new HashMap<>(); + for (Map.Entry>> versionEntry : + source.compactedSSTFileList.entrySet()) { + Map> tableMap = new HashMap<>(); + + for (Map.Entry> tableEntry : + versionEntry.getValue().entrySet()) { + tableMap.put( + tableEntry.getKey(), + Lists.newArrayList(tableEntry.getValue())); + } + + this.compactedSSTFileList.put(versionEntry.getKey(), tableMap); + } + } + + /** + * Returns whether SST is filtered for this snapshot. * @return true if SST is filtered, false otherwise */ - public boolean isSstFiltered() { + public boolean getSstFiltered() { return isSSTFiltered; } /** - * Sets whether SST is filtered. + * Sets whether SST is filtered for this snapshot. * @param sstFiltered */ public void setSstFiltered(boolean sstFiltered) { @@ -126,18 +162,18 @@ public void addUncompactedSSTFile(String table, String sstFile) { } /** - * Returns the last compaction time. + * Returns the last compaction time, in epoch milliseconds. * @return Timestamp of the last compaction */ - public Timestamp getLastCompactionTime() { + public long getLastCompactionTime() { return lastCompactionTime; } /** - * Sets the last compaction time. + * Sets the last compaction time, in epoch milliseconds. * @param lastCompactionTime Timestamp of the last compaction */ - public void setLastCompactionTime(Timestamp lastCompactionTime) { + public void setLastCompactionTime(Long lastCompactionTime) { this.lastCompactionTime = lastCompactionTime; } @@ -145,7 +181,7 @@ public void setLastCompactionTime(Timestamp lastCompactionTime) { * Returns whether the snapshot needs compaction. * @return true if the snapshot needs compaction, false otherwise */ - public boolean isNeedsCompaction() { + public boolean getNeedsCompaction() { return needsCompaction; } @@ -228,7 +264,6 @@ public void computeAndSetChecksum(Yaml yaml) throws IOException { /** * Computes SHA-256 hash for a given string. - * * @param data String data for which checksum needs to be calculated * @return SHA-256 checksum as hex string * @throws IOException If checksum calculation fails diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java index c3b25f3da162..bb867b1fb5d5 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java @@ -23,8 +23,6 @@ import java.io.IOException; import java.io.InputStream; import java.nio.file.Files; -import java.sql.Timestamp; -import java.time.Instant; import java.util.List; import java.util.Map; import java.util.Set; @@ -49,14 +47,13 @@ import org.yaml.snakeyaml.representer.Representer; /** - * Class for creating and reading snapshot data YAML files. - * Checksum of the YAML fields are computed and stored in the YAML file. + * Class for creating and reading snapshot local properties / data YAML files. + * Checksum of the YAML fields are computed and stored in the YAML file transparently to callers. * Inspired by {@link org.apache.hadoop.ozone.container.common.impl.ContainerDataYaml} */ public final class OmSnapshotLocalDataYaml { - private static final Logger LOG = - LoggerFactory.getLogger(OmSnapshotLocalDataYaml.class); + private static final Logger LOG = LoggerFactory.getLogger(OmSnapshotLocalDataYaml.class); public static final Tag SNAPSHOT_YAML_TAG = new Tag("OmSnapshotLocalData"); @@ -67,10 +64,10 @@ private OmSnapshotLocalDataYaml() { /** * Creates a snapshot data file in YAML format. */ - public static void createSnapshotFile(OmSnapshotLocalData snapshotData, - File snapshotFile) throws IOException { + public static void createSnapshotLocalDataFile(OmSnapshotLocalData snapshotData, File snapshotFile) + throws IOException { // Create Yaml - final Yaml yaml = getYamlForSnapshotData(); + final Yaml yaml = getYamlForSnapshotLocalData(); // Compute Checksum and update SnapshotData snapshotData.computeAndSetChecksum(yaml); @@ -79,33 +76,28 @@ public static void createSnapshotFile(OmSnapshotLocalData snapshotData, } /** - * Read the YAML file, and return snapshotData. - * + * Read the YAML file, and return OmSnapshotLocalData instance. * @throws IOException */ - public static OmSnapshotLocalData readSnapshotFile(File snapshotFile) + public static OmSnapshotLocalData readSnapshotLocalDataFile(File snapshotFile) throws IOException { Preconditions.checkNotNull(snapshotFile, "snapshotFile cannot be null"); - try (InputStream inputFileStream = Files.newInputStream( - snapshotFile.toPath())) { + try (InputStream inputFileStream = Files.newInputStream(snapshotFile.toPath())) { return readSnapshot(inputFileStream); } } /** - * Read the YAML file content, and return snapshotData. - * + * Read the YAML file content byte array, and return OmSnapshotLocalData instance. * @throws IOException */ public static OmSnapshotLocalData readSnapshot(byte[] snapshotFileContent) throws IOException { - return readSnapshot( - new ByteArrayInputStream(snapshotFileContent)); + return readSnapshot(new ByteArrayInputStream(snapshotFileContent)); } /** - * Read the YAML content, and return snapshotData. - * + * Read the YAML content InputStream, and return OmSnapshotLocalData instance. * @throws IOException */ public static OmSnapshotLocalData readSnapshot(InputStream input) @@ -115,11 +107,10 @@ public static OmSnapshotLocalData readSnapshot(InputStream input) propertyUtils.setBeanAccess(BeanAccess.FIELD); propertyUtils.setAllowReadOnlyProperties(true); - Representer representer = new SnapshotDataRepresenter( - OmSnapshotLocalData.YAML_FIELDS); + Representer representer = new SnapshotLocalDataRepresenter(OmSnapshotLocalData.YAML_FIELDS); representer.setPropertyUtils(propertyUtils); - SafeConstructor snapshotDataConstructor = new SnapshotDataConstructor(); + SafeConstructor snapshotDataConstructor = new SnapshotLocalDataConstructor(); Yaml yaml = new Yaml(snapshotDataConstructor, representer); yaml.setBeanAccess(BeanAccess.FIELD); @@ -141,22 +132,61 @@ public static OmSnapshotLocalData readSnapshot(InputStream input) return snapshotData; } + /** + * Verifies the checksum of the snapshot data. + * @param snapshotData The snapshot data to verify + * @return true if the checksum is valid, false otherwise + * @throws IOException if there's an error computing the checksum + */ + public static boolean verifyChecksum(OmSnapshotLocalData snapshotData) + throws IOException { + Preconditions.checkNotNull(snapshotData, "snapshotData cannot be null"); + + // Get the stored checksum + String storedChecksum = snapshotData.getChecksum(); + if (storedChecksum == null) { + LOG.warn("No checksum found in snapshot data for verification"); + return false; + } + + // Create a copy of the snapshot data for computing checksum + OmSnapshotLocalData snapshotDataCopy = new OmSnapshotLocalData(snapshotData); + + // Clear the existing checksum in the copy + snapshotDataCopy.setChecksum(null); + + // Get the YAML representation + final Yaml yaml = getYamlForSnapshotLocalData(); + + // Compute new checksum + snapshotDataCopy.computeAndSetChecksum(yaml); + + // Compare the stored and computed checksums + String computedChecksum = snapshotDataCopy.getChecksum(); + boolean isValid = storedChecksum.equals(computedChecksum); + + if (!isValid) { + LOG.warn("Checksum verification failed for snapshot local data. " + + "Stored: {}, Computed: {}", storedChecksum, computedChecksum); + } + + return isValid; + } + /** * Returns a Yaml representation of the snapshot properties. - * * @return Yaml representation of snapshot properties */ - public static Yaml getYamlForSnapshotData() { + public static Yaml getYamlForSnapshotLocalData() { PropertyUtils propertyUtils = new PropertyUtils(); propertyUtils.setBeanAccess(BeanAccess.FIELD); propertyUtils.setAllowReadOnlyProperties(true); - Representer representer = new SnapshotDataRepresenter( - OmSnapshotLocalData.YAML_FIELDS); + Representer representer = new SnapshotLocalDataRepresenter(OmSnapshotLocalData.YAML_FIELDS); representer.setPropertyUtils(propertyUtils); representer.addClassTag(OmSnapshotLocalData.class, SNAPSHOT_YAML_TAG); - SafeConstructor snapshotDataConstructor = new SnapshotDataConstructor(); + SafeConstructor snapshotDataConstructor = new SnapshotLocalDataConstructor(); return new Yaml(snapshotDataConstructor, representer); } @@ -164,25 +194,19 @@ public static Yaml getYamlForSnapshotData() { /** * Representer class to define which fields need to be stored in yaml file. */ - private static class SnapshotDataRepresenter extends Representer { + private static class SnapshotLocalDataRepresenter extends Representer { private List yamlFields; - SnapshotDataRepresenter(List yamlFields) { + SnapshotLocalDataRepresenter(List yamlFields) { super(new DumperOptions()); this.yamlFields = yamlFields; - - // Add custom representer for Timestamp to output epoch milliseconds - representers.put(Timestamp.class, data -> { - Timestamp timestamp = (Timestamp) data; - return represent(timestamp.getTime()); - }); } @Override - protected Set getProperties(Class type) { + protected Set getProperties(Class type) { Set set = super.getProperties(type); - Set filtered = new TreeSet(); + Set filtered = new TreeSet<>(); if (type.equals(OmSnapshotLocalData.class)) { // filter properties @@ -211,15 +235,14 @@ protected NodeTuple representJavaBeanProperty( /** * Constructor class for OmSnapshotLocalData, which will be used by Yaml. */ - private static class SnapshotDataConstructor extends SafeConstructor { - SnapshotDataConstructor() { + private static class SnapshotLocalDataConstructor extends SafeConstructor { + SnapshotLocalDataConstructor() { super(new LoaderOptions()); //Adding our own specific constructors for tags. - this.yamlConstructors.put( - SNAPSHOT_YAML_TAG, new ConstructSnapshotData()); + this.yamlConstructors.put(SNAPSHOT_YAML_TAG, new ConstructSnapshotLocalData()); } - private final class ConstructSnapshotData extends AbstractConstruct { + private final class ConstructSnapshotLocalData extends AbstractConstruct { @SuppressWarnings("unchecked") @Override public Object construct(Node node) { @@ -229,43 +252,19 @@ public Object construct(Node node) { OmSnapshotLocalData snapshotData = new OmSnapshotLocalData(); // Set fields from parsed YAML - snapshotData.setSstFiltered((Boolean) nodes.getOrDefault( - OzoneConsts.IS_SST_FILTERED, false)); + snapshotData.setSstFiltered((Boolean) nodes.getOrDefault(OzoneConsts.IS_SST_FILTERED, false)); Map> uncompactedSSTFileList = - (Map>) nodes.get( - OzoneConsts.UNCOMPACTED_SST_FILE_LIST); + (Map>) nodes.get(OzoneConsts.UNCOMPACTED_SST_FILE_LIST); if (uncompactedSSTFileList != null) { snapshotData.setUncompactedSSTFileList(uncompactedSSTFileList); } - Object lastCompactionTimeObj = nodes.get( - OzoneConsts.LAST_COMPACTION_TIME); - if (lastCompactionTimeObj != null) { - if (lastCompactionTimeObj instanceof Long) { - snapshotData.setLastCompactionTime( - new Timestamp((Long) lastCompactionTimeObj)); - } else if (lastCompactionTimeObj instanceof Timestamp) { - snapshotData.setLastCompactionTime( - (Timestamp) lastCompactionTimeObj); - } else if (lastCompactionTimeObj instanceof String) { - // Handle timestamp as ISO-8601 string (common in YAML) - try { - Instant instant = Instant.parse((String) lastCompactionTimeObj); - snapshotData.setLastCompactionTime(new Timestamp(instant.toEpochMilli())); - } catch (Exception e) { - // If parsing fails, use default timestamp - snapshotData.setLastCompactionTime(new Timestamp(0)); - } - } - } - - snapshotData.setNeedsCompaction((Boolean) nodes.getOrDefault( - OzoneConsts.NEEDS_COMPACTION, false)); + snapshotData.setLastCompactionTime((Long) nodes.getOrDefault(OzoneConsts.LAST_COMPACTION_TIME, -1L)); + snapshotData.setNeedsCompaction((Boolean) nodes.getOrDefault(OzoneConsts.NEEDS_COMPACTION, false)); Map>> compactedSSTFileList = - (Map>>) nodes.get( - OzoneConsts.COMPACTED_SST_FILE_LIST); + (Map>>) nodes.get(OzoneConsts.COMPACTED_SST_FILE_LIST); if (compactedSSTFileList != null) { snapshotData.setCompactedSSTFileList(compactedSSTFileList); } diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java index 72f6841f6295..98c1c6eba069 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java @@ -25,7 +25,6 @@ import java.io.File; import java.io.IOException; import java.nio.charset.Charset; -import java.sql.Timestamp; import java.time.Instant; import java.util.List; import java.util.Map; @@ -37,7 +36,6 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.yaml.snakeyaml.Yaml; /** * This class tests creating and reading snapshot data YAML files. @@ -60,9 +58,9 @@ public void cleanup() { } /** - * Creates a snapshot data YAML file. + * Creates a snapshot local data YAML file. */ - private File createSnapshotDataFile(String snapshotName) throws IOException { + private File createSnapshotLocalDataFile(String snapshotName) throws IOException { String yamlFilePath = snapshotName + ".yaml"; OmSnapshotLocalData snapshotData = new OmSnapshotLocalData(); @@ -74,7 +72,7 @@ private File createSnapshotDataFile(String snapshotName) throws IOException { snapshotData.addUncompactedSSTFile("table2", "sst3"); // Set last compaction time - snapshotData.setLastCompactionTime(new Timestamp(NOW.toEpochMilli())); + snapshotData.setLastCompactionTime(NOW.toEpochMilli()); // Set needs compaction flag snapshotData.setNeedsCompaction(true); @@ -87,7 +85,7 @@ private File createSnapshotDataFile(String snapshotName) throws IOException { File yamlFile = new File(testRoot, yamlFilePath); // Create YAML file with SnapshotData - OmSnapshotLocalDataYaml.createSnapshotFile(snapshotData, yamlFile); + OmSnapshotLocalDataYaml.createSnapshotLocalDataFile(snapshotData, yamlFile); // Check YAML file exists assertTrue(yamlFile.exists()); @@ -96,14 +94,14 @@ private File createSnapshotDataFile(String snapshotName) throws IOException { } @Test - public void testCreateSnapshotDataFile() throws IOException { - File yamlFile = createSnapshotDataFile("snapshot1"); + public void testCreateSnapshotLocalDataFile() throws IOException { + File yamlFile = createSnapshotLocalDataFile("snapshot1"); - // Read from YAML file, and verify data - OmSnapshotLocalData snapshotData = - OmSnapshotLocalDataYaml.readSnapshotFile(yamlFile); + // Read from YAML file + OmSnapshotLocalData snapshotData = OmSnapshotLocalDataYaml.readSnapshotLocalDataFile(yamlFile); - assertTrue(snapshotData.isSstFiltered()); + // Verify fields + assertTrue(snapshotData.getSstFiltered()); Map> uncompactedFiles = snapshotData.getUncompactedSSTFileList(); assertEquals(2, uncompactedFiles.size()); @@ -113,8 +111,8 @@ public void testCreateSnapshotDataFile() throws IOException { assertTrue(uncompactedFiles.get("table1").contains("sst2")); assertTrue(uncompactedFiles.get("table2").contains("sst3")); - assertEquals(NOW.toEpochMilli(), snapshotData.getLastCompactionTime().getTime()); - assertTrue(snapshotData.isNeedsCompaction()); + assertEquals(NOW.toEpochMilli(), snapshotData.getLastCompactionTime()); + assertTrue(snapshotData.getNeedsCompaction()); Map>> compactedFiles = snapshotData.getCompactedSSTFileList(); assertEquals(2, compactedFiles.size()); @@ -129,11 +127,11 @@ public void testCreateSnapshotDataFile() throws IOException { @Test public void testUpdateSnapshotDataFile() throws IOException { - File yamlFile = createSnapshotDataFile("snapshot2"); + File yamlFile = createSnapshotLocalDataFile("snapshot2"); // Read from YAML file OmSnapshotLocalData snapshotData = - OmSnapshotLocalDataYaml.readSnapshotFile(yamlFile); + OmSnapshotLocalDataYaml.readSnapshotLocalDataFile(yamlFile); // Update snapshot data snapshotData.setSstFiltered(false); @@ -142,14 +140,14 @@ public void testUpdateSnapshotDataFile() throws IOException { snapshotData.addCompactedSSTFile(3, "table3", "compacted-sst4"); // Write updated data back to file - OmSnapshotLocalDataYaml.createSnapshotFile(snapshotData, yamlFile); + OmSnapshotLocalDataYaml.createSnapshotLocalDataFile(snapshotData, yamlFile); // Read back the updated data - snapshotData = OmSnapshotLocalDataYaml.readSnapshotFile(yamlFile); + snapshotData = OmSnapshotLocalDataYaml.readSnapshotLocalDataFile(yamlFile); // Verify updated data - assertThat(snapshotData.isSstFiltered()).isFalse(); - assertThat(snapshotData.isNeedsCompaction()).isFalse(); + assertThat(snapshotData.getSstFiltered()).isFalse(); + assertThat(snapshotData.getNeedsCompaction()).isFalse(); Map> uncompactedFiles = snapshotData.getUncompactedSSTFileList(); assertEquals(3, uncompactedFiles.size()); @@ -169,18 +167,17 @@ public void testEmptyFile() throws IOException { assertTrue(emptyFile.createNewFile()); IOException ex = assertThrows(IOException.class, () -> - OmSnapshotLocalDataYaml.readSnapshotFile(emptyFile)); + OmSnapshotLocalDataYaml.readSnapshotLocalDataFile(emptyFile)); assertThat(ex).hasMessageContaining("Failed to load snapshot file. File is empty."); } @Test public void testChecksum() throws IOException { - File yamlFile = createSnapshotDataFile("snapshot3"); + File yamlFile = createSnapshotLocalDataFile("snapshot3"); // Read from YAML file - OmSnapshotLocalData snapshotData = - OmSnapshotLocalDataYaml.readSnapshotFile(yamlFile); + OmSnapshotLocalData snapshotData = OmSnapshotLocalDataYaml.readSnapshotLocalDataFile(yamlFile); // Get the original checksum String originalChecksum = snapshotData.getChecksum(); @@ -188,43 +185,12 @@ public void testChecksum() throws IOException { // Verify the checksum is not null or empty assertThat(originalChecksum).isNotNull().isNotEmpty(); - // Save the current timestamp value - Timestamp originalTimestamp = snapshotData.getLastCompactionTime(); - - // Recompute the checksum - Yaml yaml = OmSnapshotLocalDataYaml.getYamlForSnapshotData(); - snapshotData.computeAndSetChecksum(yaml); - - // Get the newly computed checksum - String recomputedChecksum = snapshotData.getChecksum(); - - // Create a new snapshot with the same data to verify checksum calculation - OmSnapshotLocalData newSnapshot = new OmSnapshotLocalData(); - newSnapshot.setSstFiltered(snapshotData.isSstFiltered()); - newSnapshot.setUncompactedSSTFileList(snapshotData.getUncompactedSSTFileList()); - newSnapshot.setLastCompactionTime(originalTimestamp); - newSnapshot.setNeedsCompaction(snapshotData.isNeedsCompaction()); - newSnapshot.setCompactedSSTFileList(snapshotData.getCompactedSSTFileList()); - - // Compute checksum for the new snapshot - newSnapshot.computeAndSetChecksum(yaml); - - // Verify the checksum of the new snapshot matches the recomputed one - assertEquals(recomputedChecksum, newSnapshot.getChecksum()); - - // Modify data and verify checksum changes - newSnapshot.addUncompactedSSTFile("table4", "sst5"); - newSnapshot.computeAndSetChecksum(yaml); - - assertThat(newSnapshot.getChecksum()) - .isNotNull() - .isNotEmpty() - .isNotEqualTo(recomputedChecksum); + OmSnapshotLocalDataYaml.verifyChecksum(snapshotData); } @Test public void testYamlContainsAllFields() throws IOException { - File yamlFile = createSnapshotDataFile("snapshot4"); + File yamlFile = createSnapshotLocalDataFile("snapshot4"); String content = FileUtils.readFileToString(yamlFile, Charset.defaultCharset()); From f68529712e6d23602e1e78437f1d2b2a1dcf6ba9 Mon Sep 17 00:00:00 2001 From: Siyao Meng <50227127+smengcl@users.noreply.github.com> Date: Thu, 12 Jun 2025 07:59:33 -0700 Subject: [PATCH 12/20] Fix method name --- .../apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java index bb867b1fb5d5..43375ea09e47 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java @@ -83,7 +83,7 @@ public static OmSnapshotLocalData readSnapshotLocalDataFile(File snapshotFile) throws IOException { Preconditions.checkNotNull(snapshotFile, "snapshotFile cannot be null"); try (InputStream inputFileStream = Files.newInputStream(snapshotFile.toPath())) { - return readSnapshot(inputFileStream); + return readSnapshotLocalData(inputFileStream); } } @@ -91,16 +91,16 @@ public static OmSnapshotLocalData readSnapshotLocalDataFile(File snapshotFile) * Read the YAML file content byte array, and return OmSnapshotLocalData instance. * @throws IOException */ - public static OmSnapshotLocalData readSnapshot(byte[] snapshotFileContent) + public static OmSnapshotLocalData readSnapshotLocalData(byte[] snapshotFileContent) throws IOException { - return readSnapshot(new ByteArrayInputStream(snapshotFileContent)); + return readSnapshotLocalData(new ByteArrayInputStream(snapshotFileContent)); } /** * Read the YAML content InputStream, and return OmSnapshotLocalData instance. * @throws IOException */ - public static OmSnapshotLocalData readSnapshot(InputStream input) + public static OmSnapshotLocalData readSnapshotLocalData(InputStream input) throws IOException { OmSnapshotLocalData snapshotData; PropertyUtils propertyUtils = new PropertyUtils(); From 788f3bc50aec11372dfe5a077618c7cb7b0b02f8 Mon Sep 17 00:00:00 2001 From: Siyao Meng <50227127+smengcl@users.noreply.github.com> Date: Thu, 12 Jun 2025 08:11:56 -0700 Subject: [PATCH 13/20] Test: Remove unused variable --- .../org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java index 98c1c6eba069..d1273f2caaf6 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java @@ -31,7 +31,6 @@ import org.apache.commons.io.FileUtils; import org.apache.hadoop.fs.FileSystemTestHelper; import org.apache.hadoop.fs.FileUtil; -import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.ozone.OzoneConsts; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; @@ -45,7 +44,6 @@ public class TestOmSnapshotLocalDataYaml { private static String testRoot = new FileSystemTestHelper().getTestRootDir(); private static final Instant NOW = Instant.now(); - private OzoneConfiguration conf = new OzoneConfiguration(); @BeforeEach public void setUp() { From efb4a4e3c50d840b84fde1b37590aa4ef1b56404 Mon Sep 17 00:00:00 2001 From: Siyao Meng <50227127+smengcl@users.noreply.github.com> Date: Fri, 13 Jun 2025 11:40:35 -0700 Subject: [PATCH 14/20] Make `OmSnapshotLocalData` abstract; clean up ser-de method in `OmSnapshotLocalDataYaml` --- .../hadoop/ozone/om/OmSnapshotLocalData.java | 4 +- .../ozone/om/OmSnapshotLocalDataYaml.java | 160 ++++++++++-------- .../ozone/om/TestOmSnapshotLocalDataYaml.java | 50 +++--- 3 files changed, 113 insertions(+), 101 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalData.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalData.java index c37c0abf562e..c933e53cb486 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalData.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalData.java @@ -32,9 +32,9 @@ /** * OmSnapshotLocalData is the in-memory representation of snapshot local metadata. - * Inspired by {@link org.apache.hadoop.ozone.container.common.impl.ContainerData} + * Inspired by org.apache.hadoop.ozone.container.common.impl.ContainerData */ -public class OmSnapshotLocalData { +public abstract class OmSnapshotLocalData { // Checksum of the YAML representation private String checksum; diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java index 43375ea09e47..8d0054a4963d 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java @@ -18,7 +18,6 @@ package org.apache.hadoop.ozone.om; import com.google.common.base.Preconditions; -import java.io.ByteArrayInputStream; import java.io.File; import java.io.IOException; import java.io.InputStream; @@ -49,87 +48,27 @@ /** * Class for creating and reading snapshot local properties / data YAML files. * Checksum of the YAML fields are computed and stored in the YAML file transparently to callers. - * Inspired by {@link org.apache.hadoop.ozone.container.common.impl.ContainerDataYaml} + * Inspired by org.apache.hadoop.ozone.container.common.impl.ContainerDataYaml */ -public final class OmSnapshotLocalDataYaml { +public final class OmSnapshotLocalDataYaml extends OmSnapshotLocalData { private static final Logger LOG = LoggerFactory.getLogger(OmSnapshotLocalDataYaml.class); public static final Tag SNAPSHOT_YAML_TAG = new Tag("OmSnapshotLocalData"); - private OmSnapshotLocalDataYaml() { - - } - - /** - * Creates a snapshot data file in YAML format. - */ - public static void createSnapshotLocalDataFile(OmSnapshotLocalData snapshotData, File snapshotFile) - throws IOException { - // Create Yaml - final Yaml yaml = getYamlForSnapshotLocalData(); - // Compute Checksum and update SnapshotData - snapshotData.computeAndSetChecksum(yaml); - - // Write the SnapshotData with checksum to Yaml file. - YamlUtils.dump(yaml, snapshotData, snapshotFile, LOG); - } - /** - * Read the YAML file, and return OmSnapshotLocalData instance. - * @throws IOException + * Creates a new OmSnapshotLocalDataYaml with default values. */ - public static OmSnapshotLocalData readSnapshotLocalDataFile(File snapshotFile) - throws IOException { - Preconditions.checkNotNull(snapshotFile, "snapshotFile cannot be null"); - try (InputStream inputFileStream = Files.newInputStream(snapshotFile.toPath())) { - return readSnapshotLocalData(inputFileStream); - } + public OmSnapshotLocalDataYaml() { + super(); } /** - * Read the YAML file content byte array, and return OmSnapshotLocalData instance. - * @throws IOException + * Copy constructor to create a deep copy. + * @param source The source OmSnapshotLocalData to copy from */ - public static OmSnapshotLocalData readSnapshotLocalData(byte[] snapshotFileContent) - throws IOException { - return readSnapshotLocalData(new ByteArrayInputStream(snapshotFileContent)); - } - - /** - * Read the YAML content InputStream, and return OmSnapshotLocalData instance. - * @throws IOException - */ - public static OmSnapshotLocalData readSnapshotLocalData(InputStream input) - throws IOException { - OmSnapshotLocalData snapshotData; - PropertyUtils propertyUtils = new PropertyUtils(); - propertyUtils.setBeanAccess(BeanAccess.FIELD); - propertyUtils.setAllowReadOnlyProperties(true); - - Representer representer = new SnapshotLocalDataRepresenter(OmSnapshotLocalData.YAML_FIELDS); - representer.setPropertyUtils(propertyUtils); - - SafeConstructor snapshotDataConstructor = new SnapshotLocalDataConstructor(); - - Yaml yaml = new Yaml(snapshotDataConstructor, representer); - yaml.setBeanAccess(BeanAccess.FIELD); - - try { - snapshotData = yaml.load(input); - } catch (YAMLException ex) { - // Unchecked exception. Convert to IOException - throw new IOException(ex); - } - - if (snapshotData == null) { - // If Yaml#load returned null, then the file is empty. This is valid yaml - // but considered an error in this case since we have lost data about - // the snapshot. - throw new IOException("Failed to load snapshot file. File is empty."); - } - - return snapshotData; + public OmSnapshotLocalDataYaml(OmSnapshotLocalData source) { + super(source); } /** @@ -150,7 +89,7 @@ public static boolean verifyChecksum(OmSnapshotLocalData snapshotData) } // Create a copy of the snapshot data for computing checksum - OmSnapshotLocalData snapshotDataCopy = new OmSnapshotLocalData(snapshotData); + OmSnapshotLocalDataYaml snapshotDataCopy = new OmSnapshotLocalDataYaml(snapshotData); // Clear the existing checksum in the copy snapshotDataCopy.setChecksum(null); @@ -184,7 +123,7 @@ public static Yaml getYamlForSnapshotLocalData() { Representer representer = new SnapshotLocalDataRepresenter(OmSnapshotLocalData.YAML_FIELDS); representer.setPropertyUtils(propertyUtils); - representer.addClassTag(OmSnapshotLocalData.class, SNAPSHOT_YAML_TAG); + representer.addClassTag(OmSnapshotLocalDataYaml.class, SNAPSHOT_YAML_TAG); SafeConstructor snapshotDataConstructor = new SnapshotLocalDataConstructor(); @@ -208,7 +147,7 @@ protected Set getProperties(Class type) { Set set = super.getProperties(type); Set filtered = new TreeSet<>(); - if (type.equals(OmSnapshotLocalData.class)) { + if (type.equals(OmSnapshotLocalDataYaml.class)) { // filter properties for (Property prop : set) { String name = prop.getName(); @@ -249,7 +188,7 @@ public Object construct(Node node) { MappingNode mnode = (MappingNode) node; Map nodes = constructMapping(mnode); - OmSnapshotLocalData snapshotData = new OmSnapshotLocalData(); + OmSnapshotLocalDataYaml snapshotData = new OmSnapshotLocalDataYaml(); // Set fields from parsed YAML snapshotData.setSstFiltered((Boolean) nodes.getOrDefault(OzoneConsts.IS_SST_FILTERED, false)); @@ -278,4 +217,77 @@ public Object construct(Node node) { } } } + + /** + * Returns the YAML representation of this object as a String + * (without triggering checksum computation or persistence). + * @return YAML string representation + */ + public String getYaml() { + final Yaml yaml = getYamlForSnapshotLocalData(); + return yaml.dump(this); + } + + /** + * Computes checksum (stored in this object), and writes this object to a YAML file. + * @param yamlFile The file to write to + * @throws IOException If there's an error writing to the file + */ + public void writeToYaml(File yamlFile) throws IOException { + // Create Yaml + final Yaml yaml = getYamlForSnapshotLocalData(); + // Compute Checksum and update SnapshotData + computeAndSetChecksum(yaml); + // Write the SnapshotData with checksum to Yaml file. + YamlUtils.dump(yaml, this, yamlFile, LOG); + } + + /** + * Creates a OmSnapshotLocalDataYaml instance from a YAML file. + * @param yamlFile The YAML file to read from + * @return A new OmSnapshotLocalDataYaml instance + * @throws IOException If there's an error reading the file + */ + public static OmSnapshotLocalDataYaml getFromYamlFile(File yamlFile) throws IOException { + Preconditions.checkNotNull(yamlFile, "yamlFile cannot be null"); + try (InputStream inputFileStream = Files.newInputStream(yamlFile.toPath())) { + return getFromYamlStream(inputFileStream); + } + } + + /** + * Read the YAML content InputStream, and return OmSnapshotLocalDataYaml instance. + * @throws IOException + */ + public static OmSnapshotLocalDataYaml getFromYamlStream(InputStream input) + throws IOException { + OmSnapshotLocalDataYaml snapshotData; + PropertyUtils propertyUtils = new PropertyUtils(); + propertyUtils.setBeanAccess(BeanAccess.FIELD); + propertyUtils.setAllowReadOnlyProperties(true); + + Representer representer = new SnapshotLocalDataRepresenter(OmSnapshotLocalData.YAML_FIELDS); + representer.setPropertyUtils(propertyUtils); + + SafeConstructor snapshotDataConstructor = new SnapshotLocalDataConstructor(); + + Yaml yaml = new Yaml(snapshotDataConstructor, representer); + yaml.setBeanAccess(BeanAccess.FIELD); + + try { + snapshotData = yaml.load(input); + } catch (YAMLException ex) { + // Unchecked exception. Convert to IOException + throw new IOException(ex); + } + + if (snapshotData == null) { + // If Yaml#load returned null, then the file is empty. This is valid yaml + // but considered an error in this case since we have lost data about + // the snapshot. + throw new IOException("Failed to load snapshot file. File is empty."); + } + + return snapshotData; + } } diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java index d1273f2caaf6..887aa49b0831 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java @@ -58,32 +58,32 @@ public void cleanup() { /** * Creates a snapshot local data YAML file. */ - private File createSnapshotLocalDataFile(String snapshotName) throws IOException { + private File writeToYaml(String snapshotName) throws IOException { String yamlFilePath = snapshotName + ".yaml"; - OmSnapshotLocalData snapshotData = new OmSnapshotLocalData(); - snapshotData.setSstFiltered(true); + OmSnapshotLocalDataYaml dataYaml = new OmSnapshotLocalDataYaml(); + dataYaml.setSstFiltered(true); // Add some uncompacted SST files - snapshotData.addUncompactedSSTFile("table1", "sst1"); - snapshotData.addUncompactedSSTFile("table1", "sst2"); - snapshotData.addUncompactedSSTFile("table2", "sst3"); + dataYaml.addUncompactedSSTFile("table1", "sst1"); + dataYaml.addUncompactedSSTFile("table1", "sst2"); + dataYaml.addUncompactedSSTFile("table2", "sst3"); // Set last compaction time - snapshotData.setLastCompactionTime(NOW.toEpochMilli()); + dataYaml.setLastCompactionTime(NOW.toEpochMilli()); // Set needs compaction flag - snapshotData.setNeedsCompaction(true); + dataYaml.setNeedsCompaction(true); // Add some compacted SST files - snapshotData.addCompactedSSTFile(1, "table1", "compacted-sst1"); - snapshotData.addCompactedSSTFile(1, "table2", "compacted-sst2"); - snapshotData.addCompactedSSTFile(2, "table1", "compacted-sst3"); + dataYaml.addCompactedSSTFile(1, "table1", "compacted-sst1"); + dataYaml.addCompactedSSTFile(1, "table2", "compacted-sst2"); + dataYaml.addCompactedSSTFile(2, "table1", "compacted-sst3"); File yamlFile = new File(testRoot, yamlFilePath); // Create YAML file with SnapshotData - OmSnapshotLocalDataYaml.createSnapshotLocalDataFile(snapshotData, yamlFile); + OmSnapshotLocalDataYaml.writeToYaml(dataYaml, yamlFile); // Check YAML file exists assertTrue(yamlFile.exists()); @@ -92,11 +92,11 @@ private File createSnapshotLocalDataFile(String snapshotName) throws IOException } @Test - public void testCreateSnapshotLocalDataFile() throws IOException { - File yamlFile = createSnapshotLocalDataFile("snapshot1"); + public void testWriteToYaml() throws IOException { + File yamlFile = writeToYaml("snapshot1"); // Read from YAML file - OmSnapshotLocalData snapshotData = OmSnapshotLocalDataYaml.readSnapshotLocalDataFile(yamlFile); + OmSnapshotLocalDataYaml snapshotData = OmSnapshotLocalDataYaml.getFromYamlFile(yamlFile); // Verify fields assertTrue(snapshotData.getSstFiltered()); @@ -125,11 +125,11 @@ public void testCreateSnapshotLocalDataFile() throws IOException { @Test public void testUpdateSnapshotDataFile() throws IOException { - File yamlFile = createSnapshotLocalDataFile("snapshot2"); + File yamlFile = writeToYaml("snapshot2"); // Read from YAML file - OmSnapshotLocalData snapshotData = - OmSnapshotLocalDataYaml.readSnapshotLocalDataFile(yamlFile); + OmSnapshotLocalDataYaml snapshotData = + OmSnapshotLocalDataYaml.getFromYamlFile(yamlFile); // Update snapshot data snapshotData.setSstFiltered(false); @@ -138,10 +138,10 @@ public void testUpdateSnapshotDataFile() throws IOException { snapshotData.addCompactedSSTFile(3, "table3", "compacted-sst4"); // Write updated data back to file - OmSnapshotLocalDataYaml.createSnapshotLocalDataFile(snapshotData, yamlFile); + OmSnapshotLocalDataYaml.writeToYaml(snapshotData, yamlFile); // Read back the updated data - snapshotData = OmSnapshotLocalDataYaml.readSnapshotLocalDataFile(yamlFile); + snapshotData = OmSnapshotLocalDataYaml.getFromYamlFile(yamlFile); // Verify updated data assertThat(snapshotData.getSstFiltered()).isFalse(); @@ -165,17 +165,17 @@ public void testEmptyFile() throws IOException { assertTrue(emptyFile.createNewFile()); IOException ex = assertThrows(IOException.class, () -> - OmSnapshotLocalDataYaml.readSnapshotLocalDataFile(emptyFile)); + OmSnapshotLocalDataYaml.getFromYamlFile(emptyFile)); assertThat(ex).hasMessageContaining("Failed to load snapshot file. File is empty."); } @Test public void testChecksum() throws IOException { - File yamlFile = createSnapshotLocalDataFile("snapshot3"); + File yamlFile = writeToYaml("snapshot3"); // Read from YAML file - OmSnapshotLocalData snapshotData = OmSnapshotLocalDataYaml.readSnapshotLocalDataFile(yamlFile); + OmSnapshotLocalDataYaml snapshotData = OmSnapshotLocalDataYaml.getFromYamlFile(yamlFile); // Get the original checksum String originalChecksum = snapshotData.getChecksum(); @@ -183,12 +183,12 @@ public void testChecksum() throws IOException { // Verify the checksum is not null or empty assertThat(originalChecksum).isNotNull().isNotEmpty(); - OmSnapshotLocalDataYaml.verifyChecksum(snapshotData); + assertTrue(OmSnapshotLocalDataYaml.verifyChecksum(snapshotData)); } @Test public void testYamlContainsAllFields() throws IOException { - File yamlFile = createSnapshotLocalDataFile("snapshot4"); + File yamlFile = writeToYaml("snapshot4"); String content = FileUtils.readFileToString(yamlFile, Charset.defaultCharset()); From 1dfdde0dd79e814ccac87ffb1e0735eb4a4b0e32 Mon Sep 17 00:00:00 2001 From: Siyao Meng <50227127+smengcl@users.noreply.github.com> Date: Mon, 16 Jun 2025 08:14:16 -0700 Subject: [PATCH 15/20] Fix UT usage --- .../ozone/om/TestOmSnapshotLocalDataYaml.java | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java index 887aa49b0831..54e9445cf48c 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java @@ -83,7 +83,7 @@ private File writeToYaml(String snapshotName) throws IOException { File yamlFile = new File(testRoot, yamlFilePath); // Create YAML file with SnapshotData - OmSnapshotLocalDataYaml.writeToYaml(dataYaml, yamlFile); + dataYaml.writeToYaml(yamlFile); // Check YAML file exists assertTrue(yamlFile.exists()); @@ -128,31 +128,31 @@ public void testUpdateSnapshotDataFile() throws IOException { File yamlFile = writeToYaml("snapshot2"); // Read from YAML file - OmSnapshotLocalDataYaml snapshotData = + OmSnapshotLocalDataYaml dataYaml = OmSnapshotLocalDataYaml.getFromYamlFile(yamlFile); // Update snapshot data - snapshotData.setSstFiltered(false); - snapshotData.setNeedsCompaction(false); - snapshotData.addUncompactedSSTFile("table3", "sst4"); - snapshotData.addCompactedSSTFile(3, "table3", "compacted-sst4"); + dataYaml.setSstFiltered(false); + dataYaml.setNeedsCompaction(false); + dataYaml.addUncompactedSSTFile("table3", "sst4"); + dataYaml.addCompactedSSTFile(3, "table3", "compacted-sst4"); // Write updated data back to file - OmSnapshotLocalDataYaml.writeToYaml(snapshotData, yamlFile); + dataYaml.writeToYaml(yamlFile); // Read back the updated data - snapshotData = OmSnapshotLocalDataYaml.getFromYamlFile(yamlFile); + dataYaml = OmSnapshotLocalDataYaml.getFromYamlFile(yamlFile); // Verify updated data - assertThat(snapshotData.getSstFiltered()).isFalse(); - assertThat(snapshotData.getNeedsCompaction()).isFalse(); + assertThat(dataYaml.getSstFiltered()).isFalse(); + assertThat(dataYaml.getNeedsCompaction()).isFalse(); - Map> uncompactedFiles = snapshotData.getUncompactedSSTFileList(); + Map> uncompactedFiles = dataYaml.getUncompactedSSTFileList(); assertEquals(3, uncompactedFiles.size()); assertTrue(uncompactedFiles.containsKey("table3")); assertTrue(uncompactedFiles.get("table3").contains("sst4")); - Map>> compactedFiles = snapshotData.getCompactedSSTFileList(); + Map>> compactedFiles = dataYaml.getCompactedSSTFileList(); assertEquals(3, compactedFiles.size()); assertTrue(compactedFiles.containsKey(3)); assertTrue(compactedFiles.get(3).containsKey("table3")); From 4864f15aa0dbebb68c57df26c3136d12eb77a398 Mon Sep 17 00:00:00 2001 From: Siyao Meng <50227127+smengcl@users.noreply.github.com> Date: Mon, 16 Jun 2025 08:18:53 -0700 Subject: [PATCH 16/20] Restore SstFilteringService --- .../hadoop/ozone/om/SstFilteringService.java | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java index a5c34d65da50..ea46366d9182 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SstFilteringService.java @@ -71,7 +71,6 @@ public class SstFilteringService extends BackgroundService private static final int SST_FILTERING_CORE_POOL_SIZE = 1; public static final String SST_FILTERED_FILE = "sstFiltered"; - public static final String SST_FILTERED_YAML_KEY = "sstFiltered"; private static final byte[] SST_FILTERED_FILE_CONTENT = StringUtils.string2Bytes("This file holds information " + "if a particular snapshot has filtered out the relevant sst files or not.\nDO NOT add, change or delete " + "any files in this directory unless you know what you are doing.\n"); @@ -87,11 +86,8 @@ public class SstFilteringService extends BackgroundService private final BootstrapStateHandler.Lock lock = new BootstrapStateHandler.Lock(); public static boolean isSstFiltered(OzoneConfiguration ozoneConfiguration, SnapshotInfo snapshotInfo) { - // TODO: First try to read the YAML file - - // Fall back to existing checks - Path sstFilteredFile = Paths.get( - OmSnapshotManager.getSnapshotPath(ozoneConfiguration, snapshotInfo), SST_FILTERED_FILE); + Path sstFilteredFile = Paths.get(OmSnapshotManager.getSnapshotPath(ozoneConfiguration, + snapshotInfo), SST_FILTERED_FILE); return snapshotInfo.isSstFiltered() || sstFilteredFile.toFile().exists(); } @@ -123,14 +119,14 @@ public void resume() { running.set(true); } - private final class SstFilteringTask implements BackgroundTask { + private class SstFilteringTask implements BackgroundTask { private boolean isSnapshotDeleted(SnapshotInfo snapshotInfo) { return snapshotInfo == null || snapshotInfo.getSnapshotStatus() == SnapshotInfo.SnapshotStatus.SNAPSHOT_DELETED; } /** - * Marks the snapshot as SSTFiltered. + * Marks the snapshot as SSTFiltered by creating a file in snapshot directory. * @param snapshotInfo snapshotInfo * @throws IOException */ @@ -145,9 +141,8 @@ private void markSSTFilteredFlagForSnapshot(SnapshotInfo snapshotInfo) throws IO if (acquiredSnapshotLock) { String snapshotDir = OmSnapshotManager.getSnapshotPath(ozoneManager.getConfiguration(), snapshotInfo); try { - // Mark the snapshot as filtered by writing to YAML property file + // mark the snapshot as filtered by creating a file. if (Files.exists(Paths.get(snapshotDir))) { - // TODO: Write to YAML Files.write(Paths.get(snapshotDir, SST_FILTERED_FILE), SST_FILTERED_FILE_CONTENT); } } finally { From 73c1702d380a4154d807c5bce71e3456d08428e5 Mon Sep 17 00:00:00 2001 From: Siyao Meng <50227127+smengcl@users.noreply.github.com> Date: Mon, 16 Jun 2025 08:36:36 -0700 Subject: [PATCH 17/20] Remove `SnapshotLocalDataRepresenter` since we are dumping ALL fields, not selective ones. --- .../hadoop/ozone/om/OmSnapshotLocalData.java | 11 ---- .../ozone/om/OmSnapshotLocalDataYaml.java | 51 ++----------------- 2 files changed, 4 insertions(+), 58 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalData.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalData.java index c933e53cb486..70a16b3d41e0 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalData.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalData.java @@ -27,7 +27,6 @@ import java.util.List; import java.util.Map; import org.apache.commons.codec.digest.DigestUtils; -import org.apache.hadoop.ozone.OzoneConsts; import org.yaml.snakeyaml.Yaml; /** @@ -55,16 +54,6 @@ public abstract class OmSnapshotLocalData { // Map> private Map>> compactedSSTFileList; - // Common Fields that need to be stored in the yaml file - public static final List YAML_FIELDS = - Collections.unmodifiableList(Lists.newArrayList( - OzoneConsts.CHECKSUM, - OzoneConsts.IS_SST_FILTERED, - OzoneConsts.UNCOMPACTED_SST_FILE_LIST, - OzoneConsts.LAST_COMPACTION_TIME, - OzoneConsts.NEEDS_COMPACTION, - OzoneConsts.COMPACTED_SST_FILE_LIST)); - public static final Charset CHARSET_ENCODING = StandardCharsets.UTF_8; private static final String DUMMY_CHECKSUM = new String(new byte[64], CHARSET_ENCODING); diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java index 8d0054a4963d..0b829876c146 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java @@ -24,8 +24,6 @@ import java.nio.file.Files; import java.util.List; import java.util.Map; -import java.util.Set; -import java.util.TreeSet; import org.apache.hadoop.hdds.server.YamlUtils; import org.apache.hadoop.ozone.OzoneConsts; import org.slf4j.Logger; @@ -37,11 +35,9 @@ import org.yaml.snakeyaml.constructor.SafeConstructor; import org.yaml.snakeyaml.error.YAMLException; import org.yaml.snakeyaml.introspector.BeanAccess; -import org.yaml.snakeyaml.introspector.Property; import org.yaml.snakeyaml.introspector.PropertyUtils; import org.yaml.snakeyaml.nodes.MappingNode; import org.yaml.snakeyaml.nodes.Node; -import org.yaml.snakeyaml.nodes.NodeTuple; import org.yaml.snakeyaml.nodes.Tag; import org.yaml.snakeyaml.representer.Representer; @@ -121,7 +117,8 @@ public static Yaml getYamlForSnapshotLocalData() { propertyUtils.setBeanAccess(BeanAccess.FIELD); propertyUtils.setAllowReadOnlyProperties(true); - Representer representer = new SnapshotLocalDataRepresenter(OmSnapshotLocalData.YAML_FIELDS); + DumperOptions options = new DumperOptions(); + Representer representer = new Representer(options); representer.setPropertyUtils(propertyUtils); representer.addClassTag(OmSnapshotLocalDataYaml.class, SNAPSHOT_YAML_TAG); @@ -130,47 +127,6 @@ public static Yaml getYamlForSnapshotLocalData() { return new Yaml(snapshotDataConstructor, representer); } - /** - * Representer class to define which fields need to be stored in yaml file. - */ - private static class SnapshotLocalDataRepresenter extends Representer { - - private List yamlFields; - - SnapshotLocalDataRepresenter(List yamlFields) { - super(new DumperOptions()); - this.yamlFields = yamlFields; - } - - @Override - protected Set getProperties(Class type) { - Set set = super.getProperties(type); - Set filtered = new TreeSet<>(); - - if (type.equals(OmSnapshotLocalDataYaml.class)) { - // filter properties - for (Property prop : set) { - String name = prop.getName(); - if (yamlFields.contains(name)) { - filtered.add(prop); - } - } - } - return filtered; - } - - /** - * Omit properties with null value. - */ - @Override - protected NodeTuple representJavaBeanProperty( - Object bean, Property property, Object value, Tag tag) { - return value == null - ? null - : super.representJavaBeanProperty(bean, property, value, tag); - } - } - /** * Constructor class for OmSnapshotLocalData, which will be used by Yaml. */ @@ -266,7 +222,8 @@ public static OmSnapshotLocalDataYaml getFromYamlStream(InputStream input) propertyUtils.setBeanAccess(BeanAccess.FIELD); propertyUtils.setAllowReadOnlyProperties(true); - Representer representer = new SnapshotLocalDataRepresenter(OmSnapshotLocalData.YAML_FIELDS); + DumperOptions options = new DumperOptions(); + Representer representer = new Representer(options); representer.setPropertyUtils(propertyUtils); SafeConstructor snapshotDataConstructor = new SnapshotLocalDataConstructor(); From c9589a151b94a8c0548c0a68e50bee73c1c4d7a4 Mon Sep 17 00:00:00 2001 From: Siyao Meng <50227127+smengcl@users.noreply.github.com> Date: Mon, 16 Jun 2025 09:00:32 -0700 Subject: [PATCH 18/20] Clean up --- .../ozone/om/OmSnapshotLocalDataYaml.java | 53 +++++++++---------- 1 file changed, 26 insertions(+), 27 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java index 0b829876c146..8ed94e2d2692 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java @@ -109,26 +109,8 @@ public static boolean verifyChecksum(OmSnapshotLocalData snapshotData) } /** - * Returns a Yaml representation of the snapshot properties. - * @return Yaml representation of snapshot properties - */ - public static Yaml getYamlForSnapshotLocalData() { - PropertyUtils propertyUtils = new PropertyUtils(); - propertyUtils.setBeanAccess(BeanAccess.FIELD); - propertyUtils.setAllowReadOnlyProperties(true); - - DumperOptions options = new DumperOptions(); - Representer representer = new Representer(options); - representer.setPropertyUtils(propertyUtils); - representer.addClassTag(OmSnapshotLocalDataYaml.class, SNAPSHOT_YAML_TAG); - - SafeConstructor snapshotDataConstructor = new SnapshotLocalDataConstructor(); - - return new Yaml(snapshotDataConstructor, representer); - } - - /** - * Constructor class for OmSnapshotLocalData, which will be used by Yaml. + * Constructor class for OmSnapshotLocalData. + * This is used when parsing YAML files into OmSnapshotLocalDataYaml objects. */ private static class SnapshotLocalDataConstructor extends SafeConstructor { SnapshotLocalDataConstructor() { @@ -211,13 +193,31 @@ public static OmSnapshotLocalDataYaml getFromYamlFile(File yamlFile) throws IOEx } } + /** + * Returns a Yaml representation of the snapshot properties. + * @return Yaml representation of snapshot properties + */ + public static Yaml getYamlForSnapshotLocalData() { + PropertyUtils propertyUtils = new PropertyUtils(); + propertyUtils.setBeanAccess(BeanAccess.FIELD); + propertyUtils.setAllowReadOnlyProperties(true); + + DumperOptions options = new DumperOptions(); + Representer representer = new Representer(options); + representer.setPropertyUtils(propertyUtils); + representer.addClassTag(OmSnapshotLocalDataYaml.class, SNAPSHOT_YAML_TAG); + + SafeConstructor snapshotDataConstructor = new SnapshotLocalDataConstructor(); + return new Yaml(snapshotDataConstructor, representer); + } + /** * Read the YAML content InputStream, and return OmSnapshotLocalDataYaml instance. * @throws IOException */ - public static OmSnapshotLocalDataYaml getFromYamlStream(InputStream input) - throws IOException { - OmSnapshotLocalDataYaml snapshotData; + public static OmSnapshotLocalDataYaml getFromYamlStream(InputStream input) throws IOException { + OmSnapshotLocalDataYaml dataYaml; + PropertyUtils propertyUtils = new PropertyUtils(); propertyUtils.setBeanAccess(BeanAccess.FIELD); propertyUtils.setAllowReadOnlyProperties(true); @@ -229,22 +229,21 @@ public static OmSnapshotLocalDataYaml getFromYamlStream(InputStream input) SafeConstructor snapshotDataConstructor = new SnapshotLocalDataConstructor(); Yaml yaml = new Yaml(snapshotDataConstructor, representer); - yaml.setBeanAccess(BeanAccess.FIELD); try { - snapshotData = yaml.load(input); + dataYaml = yaml.load(input); } catch (YAMLException ex) { // Unchecked exception. Convert to IOException throw new IOException(ex); } - if (snapshotData == null) { + if (dataYaml == null) { // If Yaml#load returned null, then the file is empty. This is valid yaml // but considered an error in this case since we have lost data about // the snapshot. throw new IOException("Failed to load snapshot file. File is empty."); } - return snapshotData; + return dataYaml; } } From 4c2d2ea9919cb5145d33e2b91d9ea783520b0604 Mon Sep 17 00:00:00 2001 From: Siyao Meng <50227127+smengcl@users.noreply.github.com> Date: Tue, 24 Jun 2025 09:21:35 -0700 Subject: [PATCH 19/20] Add `version` field. --- .../org/apache/hadoop/ozone/OzoneConsts.java | 14 ++++---- .../hadoop/ozone/om/OmSnapshotLocalData.java | 27 +++++++++++++-- .../ozone/om/OmSnapshotLocalDataYaml.java | 34 ++++++++++++------- .../ozone/om/TestOmSnapshotLocalDataYaml.java | 18 ++++++---- 4 files changed, 66 insertions(+), 27 deletions(-) diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java index d383a0686d7f..c87fcc4bf062 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java @@ -202,12 +202,14 @@ public final class OzoneConsts { public static final String SCM_CONTEXT_ATTRIBUTE = "ozone.scm"; - // YAML field constants for OmSnapshotLocalData .yaml files - public static final String IS_SST_FILTERED = "isSSTFiltered"; - public static final String UNCOMPACTED_SST_FILE_LIST = "uncompactedSSTFileList"; - public static final String LAST_COMPACTION_TIME = "lastCompactionTime"; - public static final String NEEDS_COMPACTION = "needsCompaction"; - public static final String COMPACTED_SST_FILE_LIST = "compactedSSTFileList"; + // YAML field constants for OmSnapshotLocalData (thus the OM_SLD_ prefix) YAML files + public static final String OM_SLD_VERSION = "version"; + public static final String OM_SLD_CHECKSUM = "checksum"; + public static final String OM_SLD_IS_SST_FILTERED = "isSSTFiltered"; + public static final String OM_SLD_UNCOMPACTED_SST_FILE_LIST = "uncompactedSSTFileList"; + public static final String OM_SLD_LAST_COMPACTION_TIME = "lastCompactionTime"; + public static final String OM_SLD_NEEDS_COMPACTION = "needsCompaction"; + public static final String OM_SLD_COMPACTED_SST_FILE_LIST = "compactedSSTFileList"; // YAML fields for .container files public static final String CONTAINER_ID = "containerID"; diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalData.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalData.java index 70a16b3d41e0..ce127fb3a63a 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalData.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalData.java @@ -35,6 +35,9 @@ */ public abstract class OmSnapshotLocalData { + // Version of the snapshot local data. A valid version shall be greater than 0. + private int version; + // Checksum of the YAML representation private String checksum; @@ -66,6 +69,7 @@ public OmSnapshotLocalData() { this.lastCompactionTime = 0L; this.needsCompaction = false; this.compactedSSTFileList = new HashMap<>(); + this.version = 0; setChecksumTo0ByteArray(); } @@ -79,6 +83,7 @@ public OmSnapshotLocalData(OmSnapshotLocalData source) { this.lastCompactionTime = source.lastCompactionTime; this.needsCompaction = source.needsCompaction; this.checksum = source.checksum; + this.version = source.version; // Deep copy for uncompactedSSTFileList this.uncompactedSSTFileList = new HashMap<>(); @@ -202,12 +207,12 @@ public void setCompactedSSTFileList( /** * Adds an entry to the compacted SST file list. - * @param version Version number + * @param ver Version number (TODO: to be clarified) * @param table Table name * @param sstFile SST file name */ - public void addCompactedSSTFile(Integer version, String table, String sstFile) { - this.compactedSSTFileList.computeIfAbsent(version, k -> Maps.newHashMap()) + public void addCompactedSSTFile(Integer ver, String table, String sstFile) { + this.compactedSSTFileList.computeIfAbsent(ver, k -> Maps.newHashMap()) .computeIfAbsent(table, k -> Lists.newArrayList()) .add(sstFile); } @@ -264,4 +269,20 @@ private static String getChecksum(String data) throws IOException { throw new IOException("Unable to calculate checksum", ex); } } + + /** + * Returns the version of the snapshot local data. + * @return version + */ + public int getVersion() { + return version; + } + + /** + * Sets the version of the snapshot local data. A valid version shall be greater than 0. + * @param version version + */ + public void setVersion(int version) { + this.version = version; + } } diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java index 8ed94e2d2692..97b401d8cb9b 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalDataYaml.java @@ -126,32 +126,42 @@ public Object construct(Node node) { MappingNode mnode = (MappingNode) node; Map nodes = constructMapping(mnode); - OmSnapshotLocalDataYaml snapshotData = new OmSnapshotLocalDataYaml(); + OmSnapshotLocalDataYaml snapshotLocalData = new OmSnapshotLocalDataYaml(); + + // Set version from YAML + Integer version = (Integer) nodes.get(OzoneConsts.OM_SLD_VERSION); + // Validate version. + if (version <= 0) { + // If version is not set or invalid, log a warning, but do not throw. + LOG.warn("Invalid version ({}) detected in snapshot local data YAML. Proceed with caution.", version); + } + snapshotLocalData.setVersion(version); - // Set fields from parsed YAML - snapshotData.setSstFiltered((Boolean) nodes.getOrDefault(OzoneConsts.IS_SST_FILTERED, false)); + // Set other fields from parsed YAML + snapshotLocalData.setSstFiltered((Boolean) nodes.getOrDefault(OzoneConsts.OM_SLD_IS_SST_FILTERED, false)); Map> uncompactedSSTFileList = - (Map>) nodes.get(OzoneConsts.UNCOMPACTED_SST_FILE_LIST); + (Map>) nodes.get(OzoneConsts.OM_SLD_UNCOMPACTED_SST_FILE_LIST); if (uncompactedSSTFileList != null) { - snapshotData.setUncompactedSSTFileList(uncompactedSSTFileList); + snapshotLocalData.setUncompactedSSTFileList(uncompactedSSTFileList); } - snapshotData.setLastCompactionTime((Long) nodes.getOrDefault(OzoneConsts.LAST_COMPACTION_TIME, -1L)); - snapshotData.setNeedsCompaction((Boolean) nodes.getOrDefault(OzoneConsts.NEEDS_COMPACTION, false)); + snapshotLocalData.setLastCompactionTime( + (Long) nodes.getOrDefault(OzoneConsts.OM_SLD_LAST_COMPACTION_TIME, -1L)); + snapshotLocalData.setNeedsCompaction((Boolean) nodes.getOrDefault(OzoneConsts.OM_SLD_NEEDS_COMPACTION, false)); Map>> compactedSSTFileList = - (Map>>) nodes.get(OzoneConsts.COMPACTED_SST_FILE_LIST); + (Map>>) nodes.get(OzoneConsts.OM_SLD_COMPACTED_SST_FILE_LIST); if (compactedSSTFileList != null) { - snapshotData.setCompactedSSTFileList(compactedSSTFileList); + snapshotLocalData.setCompactedSSTFileList(compactedSSTFileList); } - String checksum = (String) nodes.get(OzoneConsts.CHECKSUM); + String checksum = (String) nodes.get(OzoneConsts.OM_SLD_CHECKSUM); if (checksum != null) { - snapshotData.setChecksum(checksum); + snapshotLocalData.setChecksum(checksum); } - return snapshotData; + return snapshotLocalData; } } } diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java index 54e9445cf48c..00fea3e36012 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLocalDataYaml.java @@ -62,6 +62,10 @@ private File writeToYaml(String snapshotName) throws IOException { String yamlFilePath = snapshotName + ".yaml"; OmSnapshotLocalDataYaml dataYaml = new OmSnapshotLocalDataYaml(); + + // Set version + dataYaml.setVersion(42); + // Set SST filtered flag dataYaml.setSstFiltered(true); // Add some uncompacted SST files @@ -99,6 +103,7 @@ public void testWriteToYaml() throws IOException { OmSnapshotLocalDataYaml snapshotData = OmSnapshotLocalDataYaml.getFromYamlFile(yamlFile); // Verify fields + assertEquals(42, snapshotData.getVersion()); assertTrue(snapshotData.getSstFiltered()); Map> uncompactedFiles = snapshotData.getUncompactedSSTFileList(); @@ -193,11 +198,12 @@ public void testYamlContainsAllFields() throws IOException { String content = FileUtils.readFileToString(yamlFile, Charset.defaultCharset()); // Verify the YAML content contains all expected fields - assertThat(content).contains(OzoneConsts.IS_SST_FILTERED); - assertThat(content).contains(OzoneConsts.UNCOMPACTED_SST_FILE_LIST); - assertThat(content).contains(OzoneConsts.LAST_COMPACTION_TIME); - assertThat(content).contains(OzoneConsts.NEEDS_COMPACTION); - assertThat(content).contains(OzoneConsts.COMPACTED_SST_FILE_LIST); - assertThat(content).contains(OzoneConsts.CHECKSUM); + assertThat(content).contains(OzoneConsts.OM_SLD_VERSION); + assertThat(content).contains(OzoneConsts.OM_SLD_CHECKSUM); + assertThat(content).contains(OzoneConsts.OM_SLD_IS_SST_FILTERED); + assertThat(content).contains(OzoneConsts.OM_SLD_UNCOMPACTED_SST_FILE_LIST); + assertThat(content).contains(OzoneConsts.OM_SLD_LAST_COMPACTION_TIME); + assertThat(content).contains(OzoneConsts.OM_SLD_NEEDS_COMPACTION); + assertThat(content).contains(OzoneConsts.OM_SLD_COMPACTED_SST_FILE_LIST); } } From cb6c469cb6db45b4888c8fb5f3c9771675b96f4b Mon Sep 17 00:00:00 2001 From: Wei-Chiu Chuang Date: Wed, 9 Jul 2025 18:30:19 -0700 Subject: [PATCH 20/20] Fix Used undeclared dependencies. Change-Id: I6f258b5f8eb08affd4438f640471d927a6599c3d --- hadoop-ozone/ozone-manager/pom.xml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hadoop-ozone/ozone-manager/pom.xml b/hadoop-ozone/ozone-manager/pom.xml index d2dcdb9d6578..14e818e85414 100644 --- a/hadoop-ozone/ozone-manager/pom.xml +++ b/hadoop-ozone/ozone-manager/pom.xml @@ -310,6 +310,10 @@ org.slf4j slf4j-api + + org.yaml + snakeyaml + org.apache.ozone hdds-docs