Copilot commented on code in PR #9140:
URL: https://github.com/apache/ozone/pull/9140#discussion_r2471657925
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/snapshot/TestOmSnapshotLocalDataManager.java:
##########
@@ -135,6 +168,392 @@ public void tearDown() throws Exception {
}
}
+ private String getReadLockMessageAcquire(UUID snapshotId) {
+ return READ_LOCK_MESSAGE_ACQUIRE + " " +
FlatResource.SNAPSHOT_LOCAL_DATA_LOCK + " " + snapshotId;
+ }
+
+ private String getReadLockMessageRelease(UUID snapshotId) {
+ return READ_LOCK_MESSAGE_UNLOCK + " " +
FlatResource.SNAPSHOT_LOCAL_DATA_LOCK + " " + snapshotId;
+ }
+
+ private String getWriteLockMessageAcquire(UUID snapshotId) {
+ return WRITE_LOCK_MESSAGE_ACQUIRE + " " +
FlatResource.SNAPSHOT_LOCAL_DATA_LOCK + " " + snapshotId;
+ }
+
+ private String getWriteLockMessageRelease(UUID snapshotId) {
+ return WRITE_LOCK_MESSAGE_UNLOCK + " " +
FlatResource.SNAPSHOT_LOCAL_DATA_LOCK + " " + snapshotId;
+ }
+
+ private HierarchicalResourceLock getHierarchicalResourceLock(FlatResource
resource, String key, boolean isWriteLock) {
+ return new HierarchicalResourceLock() {
+ @Override
+ public boolean isLockAcquired() {
+ return true;
+ }
+
+ @Override
+ public void close() {
+ if (isWriteLock) {
+ lockCapturor.add(WRITE_LOCK_MESSAGE_UNLOCK + " " + resource + " " +
key);
+ } else {
+ lockCapturor.add(READ_LOCK_MESSAGE_UNLOCK + " " + resource + " " +
key);
+ }
+ }
+ };
+ }
+
+ private void mockLockManager() throws IOException {
+ lockCapturor.clear();
+ reset(lockManager);
+ when(lockManager.acquireReadLock(any(FlatResource.class), anyString()))
+ .thenAnswer(i -> {
+ lockCapturor.add(READ_LOCK_MESSAGE_ACQUIRE + " " + i.getArgument(0)
+ " " + i.getArgument(1));
+ return getHierarchicalResourceLock(i.getArgument(0),
i.getArgument(1), false);
+ });
+ when(lockManager.acquireWriteLock(any(FlatResource.class), anyString()))
+ .thenAnswer(i -> {
+ lockCapturor.add(WRITE_LOCK_MESSAGE_ACQUIRE + " " + i.getArgument(0)
+ " " + i.getArgument(1));
+ return getHierarchicalResourceLock(i.getArgument(0),
i.getArgument(1), true);
+ });
+ }
+
+ private List<UUID> createSnapshotLocalData(OmSnapshotLocalDataManager
snapshotLocalDataManager,
+ int numberOfSnapshots) throws IOException {
+ SnapshotInfo previousSnapshotInfo = null;
+ int counter = 0;
+ Map<String, List<LiveFileMetaData>> liveFileMetaDataMap = new HashMap<>();
+ liveFileMetaDataMap.put(KEY_TABLE,
+ Lists.newArrayList(createMockLiveFileMetaData("file1.sst", KEY_TABLE,
"key1", "key2")));
+ liveFileMetaDataMap.put(FILE_TABLE,
Lists.newArrayList(createMockLiveFileMetaData("file2.sst", FILE_TABLE, "key1",
+ "key2")));
+ liveFileMetaDataMap.put(DIRECTORY_TABLE,
Lists.newArrayList(createMockLiveFileMetaData("file2.sst",
+ DIRECTORY_TABLE, "key1", "key2")));
+ liveFileMetaDataMap.put("col1",
Lists.newArrayList(createMockLiveFileMetaData("file2.sst", "col1", "key1",
+ "key2")));
+ List<UUID> snapshotIds = new ArrayList<>();
+ for (int i = 0; i < numberOfSnapshots; i++) {
+ UUID snapshotId = UUID.randomUUID();
+ SnapshotInfo snapshotInfo = createMockSnapshotInfo(snapshotId,
previousSnapshotInfo == null ? null
+ : previousSnapshotInfo.getSnapshotId());
+ mockSnapshotStore(snapshotId, liveFileMetaDataMap.values().stream()
+ .flatMap(Collection::stream).collect(Collectors.toList()));
+ snapshotLocalDataManager.createNewOmSnapshotLocalDataFile(snapshotStore,
snapshotInfo);
+ previousSnapshotInfo = snapshotInfo;
+ for (Map.Entry<String, List<LiveFileMetaData>> tableEntry :
liveFileMetaDataMap.entrySet()) {
+ String table = tableEntry.getKey();
+ tableEntry.getValue().add(createMockLiveFileMetaData("file" +
counter++ + ".sst", table, "key1", "key4"));
+ }
+ snapshotIds.add(snapshotId);
+ }
+ return snapshotIds;
+ }
+
+ private void mockSnapshotStore(UUID snapshotId, List<LiveFileMetaData>
sstFiles) throws RocksDatabaseException {
+ // Setup snapshot store mock
+ File snapshotDbLocation =
OmSnapshotManager.getSnapshotPath(omMetadataManager, snapshotId).toFile();
+ assertTrue(snapshotDbLocation.exists() || snapshotDbLocation.mkdirs());
+
+ when(snapshotStore.getDbLocation()).thenReturn(snapshotDbLocation);
+ RocksDatabase rocksDatabase = mock(RocksDatabase.class);
+ when(snapshotStore.getDb()).thenReturn(rocksDatabase);
+ when(rocksDatabase.getLiveFilesMetaData()).thenReturn(sstFiles);
+ }
+
+ /**
+ * Checks lock orders taken i.e. while reading a snapshot against the
previous snapshot.
+ * Depending on read or write locks are acquired on the snapshotId and read
lock is acquired on the previous
+ * snapshot. Once the instance is closed the read lock on previous snapshot
is released followed by releasing the
+ * lock on the snapshotId.
+ * @param read
Review Comment:
The @param 'read' lacks a description. Add a description such as '@param
read if true, acquire read lock; if false, acquire write lock'.
```suggestion
* @param read if true, acquire read lock; if false, acquire write lock
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/OmSnapshotLocalDataManager.java:
##########
@@ -217,6 +256,46 @@ private void init() throws IOException {
}
}
+ /**
+ * Acquires a write lock and provides an auto-closeable supplier for
specifying details
+ * of the lock acquisition. The lock is released when the returned supplier
is closed.
+ *
+ * @return an instance of {@code
UncheckedAutoCloseableSupplier<OMLockDetails>} representing
+ * the acquired lock details, where the lock will automatically be
released on close.
+ */
+ public UncheckedAutoCloseableSupplier<OMLockDetails> lock() {
+ this.fullLock.writeLock().lock();
+ return new UncheckedAutoCloseableSupplier<OMLockDetails>() {
+ @Override
+ public OMLockDetails get() {
+ return OMLockDetails.EMPTY_DETAILS_LOCK_ACQUIRED;
+ }
+
+ @Override
+ public void close() {
+ fullLock.writeLock().unlock();
+ }
+ };
+ }
+
+ private void validateVersionRemoval(UUID snapshotId, int version) throws
IOException {
+ LocalDataVersionNode versionNode = getVersionNode(snapshotId, version);
+ if (versionNode != null && localDataGraph.inDegree(versionNode) != 0) {
+ Set<LocalDataVersionNode> versionNodes =
localDataGraph.predecessors(versionNode);
+ throw new IOException(String.format("Cannot remove Snapshot %s with
version : %d since it still has " +
+ "predecessors : %s", snapshotId, version, versionNodes));
Review Comment:
The error message uses 'predecessors' when referring to nodes that depend on
this version. Based on the logic checking `inDegree`, these are actually
successors (dependents). Consider changing 'predecessors' to 'successors' or
'dependents' for clarity.
```suggestion
"successors : %s", snapshotId, version, versionNodes));
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/OmSnapshotLocalDataManager.java:
##########
@@ -228,6 +307,413 @@ public void close() {
}
}
+ private static final class LockDataProviderInitResult {
+ private final OmSnapshotLocalData snapshotLocalData;
+ private final HierarchicalResourceLock lock;
+ private final HierarchicalResourceLock previousLock;
+ private final UUID previousSnapshotId;
+
+ private LockDataProviderInitResult(HierarchicalResourceLock lock,
OmSnapshotLocalData snapshotLocalData,
+ HierarchicalResourceLock previousLock, UUID previousSnapshotId) {
+ this.lock = lock;
+ this.snapshotLocalData = snapshotLocalData;
+ this.previousLock = previousLock;
+ this.previousSnapshotId = previousSnapshotId;
+ }
+
+ private HierarchicalResourceLock getLock() {
+ return lock;
+ }
+
+ private HierarchicalResourceLock getPreviousLock() {
+ return previousLock;
+ }
+
+ private UUID getPreviousSnapshotId() {
+ return previousSnapshotId;
+ }
+
+ private OmSnapshotLocalData getSnapshotLocalData() {
+ return snapshotLocalData;
+ }
+ }
+
+ /**
+ * The ReadableOmSnapshotLocalDataProvider class is responsible for managing
the
+ * access and initialization of local snapshot data in a thread-safe manner.
+ * It provides mechanisms to handle snapshot data, retrieve associated
previous
+ * snapshot data, and manage lock synchronization for safe concurrent
operations.
+ *
+ * This class works with snapshot identifiers and ensures that the
appropriate
+ * local data for a given snapshot is loaded and accessible. Additionally, it
+ * maintains locking mechanisms to ensure thread-safe initialization and
access
+ * to both the current and previous snapshot local data. The implementation
also
+ * supports handling errors in the snapshot data initialization process.
+ *
+ * Key Functionalities:
+ * - Initializes and provides access to snapshot local data associated with a
+ * given snapshot identifier.
+ * - Resolves and retrieves data for the previous snapshot if applicable.
+ * - Ensures safe concurrent read operations using locking mechanisms.
+ * - Validates the integrity and consistency of snapshot data during
initialization.
+ * - Ensures that appropriate locks are released upon closing.
+ *
+ * Thread-Safety:
+ * This class utilizes locks to guarantee thread-safe operations when
accessing
+ * or modifying snapshot data. State variables relating to snapshot data are
+ * properly synchronized to ensure consistency during concurrent operations.
+ *
+ * Usage Considerations:
+ * - Ensure proper handling of exceptions while interacting with this class,
+ * particularly during initialization and cleanup.
+ * - Always invoke the {@code close()} method after usage to release
acquired locks
+ * and avoid potential deadlocks.
+ */
+ public class ReadableOmSnapshotLocalDataProvider implements AutoCloseable {
+
+ private final UUID snapshotId;
+ private final HierarchicalResourceLock lock;
+ private final HierarchicalResourceLock previousLock;
+ private final OmSnapshotLocalData snapshotLocalData;
+ private OmSnapshotLocalData previousSnapshotLocalData;
+ private volatile boolean isPreviousSnapshotLoaded = false;
+ private final UUID resolvedPreviousSnapshotId;
+
+ protected ReadableOmSnapshotLocalDataProvider(UUID snapshotId) throws
IOException {
+ this(snapshotId, true);
+ }
+
+ protected ReadableOmSnapshotLocalDataProvider(UUID snapshotId, UUID
snapIdToResolve) throws IOException {
+ this(snapshotId, true, null, snapIdToResolve, true);
+ }
+
+ protected ReadableOmSnapshotLocalDataProvider(UUID snapshotId, boolean
readLock) throws IOException {
+ this(snapshotId, readLock, null, null, false);
+ }
+
+ protected ReadableOmSnapshotLocalDataProvider(UUID snapshotId, boolean
readLock,
+ CheckedSupplier<Pair<OmSnapshotLocalData, File>, IOException>
snapshotLocalDataSupplier,
+ UUID snapshotIdToBeResolved, boolean isSnapshotToBeResolvedNullable)
throws IOException {
+ this.snapshotId = snapshotId;
+ LockDataProviderInitResult result = initialize(readLock, snapshotId,
snapshotIdToBeResolved,
+ isSnapshotToBeResolvedNullable, snapshotLocalDataSupplier);
+ this.snapshotLocalData = result.getSnapshotLocalData();
+ this.lock = result.getLock();
+ this.previousLock = result.getPreviousLock();
+ this.resolvedPreviousSnapshotId = result.getPreviousSnapshotId();
+ this.previousSnapshotLocalData = null;
+ this.isPreviousSnapshotLoaded = false;
+ }
+
+ public OmSnapshotLocalData getSnapshotLocalData() {
+ return snapshotLocalData;
+ }
+
+ public synchronized OmSnapshotLocalData getPreviousSnapshotLocalData()
throws IOException {
+ if (!isPreviousSnapshotLoaded) {
+ if (resolvedPreviousSnapshotId != null) {
+ File previousSnapshotLocalDataFile = new
File(getSnapshotLocalPropertyYamlPath(resolvedPreviousSnapshotId));
+ this.previousSnapshotLocalData =
snapshotLocalDataSerializer.load(previousSnapshotLocalDataFile);
+ }
+ this.isPreviousSnapshotLoaded = true;
+ }
+ return previousSnapshotLocalData;
+ }
+
+ private HierarchicalResourceLock acquireLock(UUID snapId, boolean
readLock) throws IOException {
+ HierarchicalResourceLock acquiredLock = readLock ?
locks.acquireReadLock(FlatResource.SNAPSHOT_LOCAL_DATA_LOCK,
+ snapId.toString()) :
locks.acquireWriteLock(FlatResource.SNAPSHOT_LOCAL_DATA_LOCK,
snapId.toString());
+ if (!acquiredLock.isLockAcquired()) {
+ throw new IOException("Unable to acquire lock for snapshotId: " +
snapId);
+ }
+ return acquiredLock;
+ }
+
+ /**
+ * Intializes the snapshot local data by acquiring the lock on the
snapshot and also acquires a read lock on the
Review Comment:
Corrected spelling of 'Intializes' to 'Initializes'.
```suggestion
* Initializes the snapshot local data by acquiring the lock on the
snapshot and also acquires a read lock on the
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotLocalData.java:
##########
@@ -274,7 +279,7 @@ public OmSnapshotLocalData copyObject() {
* maintain immutability.
*/
public static class VersionMeta implements CopyObject<VersionMeta> {
Review Comment:
The field `previousSnapshotVersion` is changed from `final` to mutable
without a clear design comment explaining why mutability is required. Consider
adding a comment explaining the use case for mutation (appears to be for
version resolution during lock acquisition).
```suggestion
public static class VersionMeta implements CopyObject<VersionMeta> {
/**
* The version of the previous snapshot. This field is mutable to allow
* version resolution during lock acquisition and other update scenarios.
* Although the class is generally intended to be immutable, mutability
* of this field is required for certain internal operations where the
* previous snapshot version may need to be updated after object
creation.
*/
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
