smengcl commented on code in PR #4909:
URL: https://github.com/apache/ozone/pull/4909#discussion_r1231605719
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotCreateRequest.java:
##########
@@ -220,17 +190,79 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager,
snapshotInfo.toAuditMap(), exception, userInfo));
if (exception == null) {
- LOG.info("Created snapshot '{}' under path '{}'",
- snapshotName, snapshotPath);
+ LOG.info("Created snapshot: '{}' with snapshotId: '{}' under path '{}'",
+ snapshotName, snapshotInfo.getSnapshotId(), snapshotPath);
omMetrics.incNumSnapshotActive();
} else {
omMetrics.incNumSnapshotCreateFails();
- LOG.error("Failed to create snapshot '{}' under path '{}'",
- snapshotName, snapshotPath);
+ LOG.error("Failed to create snapshot '{}' with snapshotId: '{}' under " +
+ "path '{}'",
+ snapshotName, snapshotInfo.getSnapshotId(), snapshotPath);
}
return omClientResponse;
}
+ /**
+ * Add snapshot to snapshot chain and snapshot cache as a single atomic
+ * operation.
+ * If it is not done as a single atomic operation, can cause data corruption
+ * if there is any failure in adding snapshot entry to cache.
+ * For example, when two threads create snapshots on different
+ * buckets.
+ * T-1: Thread-1 adds the snapshot-1 to chain first. Previous snapshot for
+ * snapshot-1 would be null (let's assume first snapshot)
+ * T-2: Thread-2 adds snapshot-2, previous would be snapshot-1
+ * T-3: Thread-1 tries to update cache with snapshot-1 and fails.
+ * T-4: Thread-2 tries to update cache with snapshot-2 and succeeds.
+ * T-5: Thread-1 removes the snapshot from chain because it failed to
+ * update the cache.
+ * Now, snapshot-2's previous is snapshot-1 but, it doesn't exist because
+ * it was removed at T-5.
+ */
+ private void addSnapshotInfoToSnapshotChainAndCache(
+ OmMetadataManagerImpl omMetadataManager,
+ long transactionLogIndex
+ ) throws IOException {
+ SnapshotChainManager snapshotChainManager =
+ omMetadataManager.getSnapshotChainManager();
+
+ // It is synchronized at SnapshotChainManager object so that this block is
+ // synchronized with OMSnapshotPurgeResponse#cleanupSnapshotChain and only
+ // one of these two operation gets executed at a time otherwise we could be
+ // in similar situation explained above if snapshot gets deleted.
+ synchronized (omMetadataManager.getSnapshotChainManager()) {
Review Comment:
Could just use:
```suggestion
synchronized (snapshotChainManager) {
```
since they point to the same instance?
--
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]
