hemantk-12 commented on code in PR #7112:
URL: https://github.com/apache/ozone/pull/7112#discussion_r1731942086
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java:
##########
@@ -101,15 +99,13 @@ public OMClientResponse
validateAndUpdateCache(OzoneManager ozoneManager, TermIn
}
SnapshotInfo nextSnapshot =
- SnapshotUtils.getNextActiveSnapshot(fromSnapshot,
snapshotChainManager, omSnapshotManager);
+ SnapshotUtils.getNextActiveSnapshot(fromSnapshot,
snapshotChainManager, ozoneManager);
Review Comment:
`ozoneManager` is correct. We had an unnecessary function
[OmSnapshotManager#getSnapshotInfo](
https://github.com/apache/ozone/pull/7112/files#diff-857b24769ac9c1c67d015bdffc1227dc2b4751de3790b9fd89269a3c46a5218aL688-L690).
It is doing nothing other than getting `OzoneManager` and passing it to
`SnapshotUtils`. We can directly pass `OzoneManager` to it.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -674,19 +675,36 @@ private ReferenceCounted<OmSnapshot> getSnapshot(String
snapshotTableKey, boolea
}
/**
- * Returns true if the snapshot is in given status.
- * @param key DB snapshot table key
- * @param status SnapshotStatus
- * @return true if the snapshot is in given status, false otherwise
+ * Returns OmSnapshot object and skips active check.
+ * This should only be used for API calls initiated by background service
e.g. purgeKeys, purgeSnapshot,
+ * snapshotMoveDeletedKeys, and SetSnapshotProperty.
*/
- public boolean isSnapshotStatus(String key,
- SnapshotInfo.SnapshotStatus status)
- throws IOException {
- return getSnapshotInfo(key).getSnapshotStatus().equals(status);
+ public ReferenceCounted<OmSnapshot> getSnapshot(UUID snapshotId) throws
IOException {
+ return snapshotCache.get(snapshotId);
}
- public SnapshotInfo getSnapshotInfo(String key) throws IOException {
- return SnapshotUtils.getSnapshotInfo(ozoneManager, key);
+ /**
+ * Returns snapshotInfo from cache if it is present in cache, otherwise it
checks RocksDB and return value from there.
+ * This should be used by SnapshotCache only.
+ * Sometimes, the follower OM node may be lagging that it gets purgeKeys or
snapshotMoveDeletedKeys from a Snapshot,
+ * and purgeSnapshot for the same Snapshot one after another. And
purgeSnapshot's validateAndUpdateCache gets
+ * executed before doubleBuffer flushes purgeKeys or snapshotMoveDeletedKeys
from that Snapshot.
+ * This should not be a case on the leader node because
SnapshotDeletingService checks that deletedTable and
+ * deletedDirectoryTable in DB don't have entries for the bucket before it
sends a purgeSnapshot on a snapshot.
+ * If that happens, and we just look into the cache, the addToBatch
operation will fail when it tries to open
+ * the DB and purgeKeys from the Snapshot because snapshot is already purged
from the SnapshotInfoTable cache.
+ * Hence, it is needed to look into the table to make sure that snapshot
exists somewhere either in cache or in DB.
+ */
+ private SnapshotInfo getSnapshotInfo(String snapshotKey) throws IOException {
+ SnapshotInfo snapshotInfo =
ozoneManager.getMetadataManager().getSnapshotInfoTable().get(snapshotKey);
+
+ if (snapshotInfo == null) {
+ snapshotInfo =
ozoneManager.getMetadataManager().getSnapshotInfoTable().getSkipCache(snapshotKey);
Review Comment:
This function is only intended for SnapshotCache in
[createCacheLoader](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java#L355).
We are not changing the original `SnapshotUtils.#getSnapshotInfo()` which
looks for value in the cache only and is used everywhere.
That's why I added a comment "This should be used by SnapshotCache only.".
I can remove the function and move this code inside `createCacheLoader` to
limit its usage if that helps.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyPurgeRequest.java:
##########
@@ -74,14 +75,12 @@ public OMClientResponse validateAndUpdateCache(OzoneManager
ozoneManager, TermIn
try {
SnapshotInfo fromSnapshotInfo = null;
if (fromSnapshot != null) {
- fromSnapshotInfo = ozoneManager.getMetadataManager()
- .getSnapshotInfoTable().get(fromSnapshot);
+ fromSnapshotInfo = SnapshotUtils.getSnapshotInfo(ozoneManager,
fromSnapshot);
Review Comment:
I haven't changed `SnapshotUtils.getSnapshotInfo()`.
I just added a private function to `OmSnapshotManager` to use for
`SnapshotCache` because it needs to know the snapshot dir to open a RocksDB
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]
