You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "hemantk-12 (via GitHub)" <gi...@apache.org> on 2024/08/07 18:28:01 UTC
[PR] HDDS-9198. Maintain local cache in OMSnapshotPurgeRequest to get updated snapshotInfo and pass the same to OMSnapshotPurgeResponse [ozone]
hemantk-12 opened a new pull request, #7045:
URL: https://github.com/apache/ozone/pull/7045
## What changes were proposed in this pull request?
Currently, we maintain two maps updatedPathPreviousAndGlobalSnapshots and updatedSnapInfos which creates an issue with batch processing because order matters. If we use one map instead of two and use that as local cache, it would solve the problem and we can still keep the batch purge operation. (More details: https://github.com/apache/ozone/pull/6491#issuecomment-2272219528)
## What is the link to the Apache JIRA
HDDS-9198
## How was this patch tested?
Existing unit and integration tests.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9198. Maintain local cache in OMSnapshotPurgeRequest to get updated snapshotInfo and pass the same to OMSnapshotPurgeResponse [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #7045:
URL: https://github.com/apache/ozone/pull/7045#discussion_r1717649173
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java:
##########
@@ -287,18 +219,25 @@ private void updateSnapshotChainAndCache(
metadataManager.getSnapshotInfoTable().addCacheEntry(
new CacheKey<>(nextPathSnapInfo.getTableKey()),
CacheValue.get(trxnLogIndex, nextPathSnapInfo));
- updatedPathPreviousAndGlobalSnapshots
- .put(nextPathSnapInfo.getTableKey(), nextPathSnapInfo);
} else if (nextGlobalSnapInfo != null) {
nextGlobalSnapInfo.setGlobalPreviousSnapshotId(
snapInfo.getGlobalPreviousSnapshotId());
metadataManager.getSnapshotInfoTable().addCacheEntry(
new CacheKey<>(nextGlobalSnapInfo.getTableKey()),
CacheValue.get(trxnLogIndex, nextGlobalSnapInfo));
- updatedPathPreviousAndGlobalSnapshots
- .put(nextGlobalSnapInfo.getTableKey(), nextGlobalSnapInfo);
}
snapshotChainManager.deleteSnapshot(snapInfo);
}
+
+ private SnapshotInfo getUpdatedSnapshotInfo(String snapshotTableKey, OMMetadataManager omMetadataManager)
+ throws IOException {
+ if (updatedSnapshotInfos.containsKey(snapshotTableKey)) {
+ return updatedSnapshotInfos.get(snapshotTableKey);
+ } else {
+ SnapshotInfo snapshotInfo = omMetadataManager.getSnapshotInfoTable().get(snapshotTableKey);
+ updatedSnapshotInfos.put(snapshotTableKey, snapshotInfo);
+ return snapshotInfo;
+ }
Review Comment:
It would be better to throw an uncheckedIOException and catch in case of exceptions. containsKey & put would do 2 lookups vs 1.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9198. Maintain local cache in OMSnapshotPurgeRequest to get updated snapshotInfo and pass the same to OMSnapshotPurgeResponse [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #7045:
URL: https://github.com/apache/ozone/pull/7045#discussion_r1722317218
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java:
##########
@@ -223,18 +219,25 @@ private void updateSnapshotChainAndCache(
metadataManager.getSnapshotInfoTable().addCacheEntry(
new CacheKey<>(nextPathSnapInfo.getTableKey()),
CacheValue.get(trxnLogIndex, nextPathSnapInfo));
- updatedPathPreviousAndGlobalSnapshots
- .put(nextPathSnapInfo.getTableKey(), nextPathSnapInfo);
} else if (nextGlobalSnapInfo != null) {
Review Comment:
Apologies for not realizing this earlier.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9198. Maintain local cache in OMSnapshotPurgeRequest to get updated snapshotInfo and pass the same to OMSnapshotPurgeResponse [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on PR #7045:
URL: https://github.com/apache/ozone/pull/7045#issuecomment-2310905869
@aswinshakil Do you wanna take a look at this patch?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9198. Maintain local cache in OMSnapshotPurgeRequest to get updated snapshotInfo and pass the same to OMSnapshotPurgeResponse [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #7045:
URL: https://github.com/apache/ozone/pull/7045#discussion_r1722495635
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java:
##########
@@ -223,18 +219,25 @@ private void updateSnapshotChainAndCache(
metadataManager.getSnapshotInfoTable().addCacheEntry(
new CacheKey<>(nextPathSnapInfo.getTableKey()),
CacheValue.get(trxnLogIndex, nextPathSnapInfo));
- updatedPathPreviousAndGlobalSnapshots
- .put(nextPathSnapInfo.getTableKey(), nextPathSnapInfo);
} else if (nextGlobalSnapInfo != null) {
Review Comment:
Yes, updated it as suggested.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9198. Maintain local cache in OMSnapshotPurgeRequest to get updated snapshotInfo and pass the same to OMSnapshotPurgeResponse [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on PR #7045:
URL: https://github.com/apache/ozone/pull/7045#issuecomment-2311259401
Thanks @swamirishi and @aswinshakil for the review.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9198. Maintain local cache in OMSnapshotPurgeRequest to get updated snapshotInfo and pass the same to OMSnapshotPurgeResponse [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #7045:
URL: https://github.com/apache/ozone/pull/7045#discussion_r1717204935
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java:
##########
@@ -287,18 +219,25 @@ private void updateSnapshotChainAndCache(
metadataManager.getSnapshotInfoTable().addCacheEntry(
new CacheKey<>(nextPathSnapInfo.getTableKey()),
CacheValue.get(trxnLogIndex, nextPathSnapInfo));
- updatedPathPreviousAndGlobalSnapshots
- .put(nextPathSnapInfo.getTableKey(), nextPathSnapInfo);
} else if (nextGlobalSnapInfo != null) {
nextGlobalSnapInfo.setGlobalPreviousSnapshotId(
snapInfo.getGlobalPreviousSnapshotId());
metadataManager.getSnapshotInfoTable().addCacheEntry(
new CacheKey<>(nextGlobalSnapInfo.getTableKey()),
CacheValue.get(trxnLogIndex, nextGlobalSnapInfo));
- updatedPathPreviousAndGlobalSnapshots
- .put(nextGlobalSnapInfo.getTableKey(), nextGlobalSnapInfo);
}
snapshotChainManager.deleteSnapshot(snapInfo);
}
+
+ private SnapshotInfo getUpdatedSnapshotInfo(String snapshotTableKey, OMMetadataManager omMetadataManager)
+ throws IOException {
+ if (updatedSnapshotInfos.containsKey(snapshotTableKey)) {
+ return updatedSnapshotInfos.get(snapshotTableKey);
+ } else {
+ SnapshotInfo snapshotInfo = omMetadataManager.getSnapshotInfoTable().get(snapshotTableKey);
+ updatedSnapshotInfos.put(snapshotTableKey, snapshotInfo);
+ return snapshotInfo;
+ }
Review Comment:
```suggestion
return updatedSnapshotInfos.computeIfAbsent(snapshotTableKey, k -> omMetadataManager.getSnapshotInfoTable().get(snapshotTableKey));
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java:
##########
@@ -87,77 +88,42 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, TermIn
try {
List<String> snapshotDbKeys = snapshotPurgeRequest
.getSnapshotDBKeysList();
- Map<String, SnapshotInfo> updatedSnapInfos = new HashMap<>();
- Map<String, SnapshotInfo> updatedPathPreviousAndGlobalSnapshots =
- new HashMap<>();
// Each snapshot purge operation does three things:
- // 1. Update the snapshot chain,
- // 2. Update the deep clean flag for the next active snapshot (So that it can be
+ // 1. Update the deep clean flag for the next active snapshot (So that it can be
// deep cleaned by the KeyDeletingService in the next run),
+ // 2. Update the snapshot chain,
// 3. Finally, purge the snapshot.
- // All of these steps have to be performed only when it acquires all the necessary
- // locks (lock on the snapshot to be purged, lock on the next active snapshot, and
- // lock on the next path and global previous snapshots). Ideally, there is no need
- // for locks for snapshot purge and can rely on OMStateMachine because OMStateMachine
- // is going to process each request sequentially.
- //
- // But there is a problem with that. After filtering unnecessary SST files for a snapshot,
- // SstFilteringService updates that snapshot's SstFilter flag. SstFilteringService cannot
- // use SetSnapshotProperty API because it runs on each OM independently and One OM does
- // not know if the snapshot has been filtered on the other OM in HA environment.
- //
- // If locks are not taken snapshot purge and SstFilteringService will cause a race condition
- // and override one's update with another.
+ // There is no need to take lock for snapshot purge as of now. We can simply rely on OMStateMachine
+ // because it executes transaction sequentially.
for (String snapTableKey : snapshotDbKeys) {
- // To acquire all the locks, a set is maintained which is keyed by snapshotTableKey.
- // snapshotTableKey is nothing but /volumeName/bucketName/snapshotName.
- // Once all the locks are acquired, it performs the three steps mentioned above and
- // release all the locks after that.
- Set<Triple<String, String, String>> lockSet = new HashSet<>(4, 1);
Review Comment:
@hemantk-12 why are we removing the locks? Don't we need to take a lock?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java:
##########
@@ -87,77 +88,42 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, TermIn
try {
List<String> snapshotDbKeys = snapshotPurgeRequest
.getSnapshotDBKeysList();
- Map<String, SnapshotInfo> updatedSnapInfos = new HashMap<>();
- Map<String, SnapshotInfo> updatedPathPreviousAndGlobalSnapshots =
- new HashMap<>();
// Each snapshot purge operation does three things:
- // 1. Update the snapshot chain,
- // 2. Update the deep clean flag for the next active snapshot (So that it can be
+ // 1. Update the deep clean flag for the next active snapshot (So that it can be
// deep cleaned by the KeyDeletingService in the next run),
+ // 2. Update the snapshot chain,
// 3. Finally, purge the snapshot.
- // All of these steps have to be performed only when it acquires all the necessary
- // locks (lock on the snapshot to be purged, lock on the next active snapshot, and
- // lock on the next path and global previous snapshots). Ideally, there is no need
- // for locks for snapshot purge and can rely on OMStateMachine because OMStateMachine
- // is going to process each request sequentially.
- //
- // But there is a problem with that. After filtering unnecessary SST files for a snapshot,
- // SstFilteringService updates that snapshot's SstFilter flag. SstFilteringService cannot
- // use SetSnapshotProperty API because it runs on each OM independently and One OM does
- // not know if the snapshot has been filtered on the other OM in HA environment.
- //
- // If locks are not taken snapshot purge and SstFilteringService will cause a race condition
- // and override one's update with another.
+ // There is no need to take lock for snapshot purge as of now. We can simply rely on OMStateMachine
+ // because it executes transaction sequentially.
for (String snapTableKey : snapshotDbKeys) {
- // To acquire all the locks, a set is maintained which is keyed by snapshotTableKey.
- // snapshotTableKey is nothing but /volumeName/bucketName/snapshotName.
- // Once all the locks are acquired, it performs the three steps mentioned above and
- // release all the locks after that.
- Set<Triple<String, String, String>> lockSet = new HashSet<>(4, 1);
Review Comment:
I believe this lock now has to become a global lockset outside the for loop & this should be only released after the entire batch operation.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9198. Maintain local cache in OMSnapshotPurgeRequest to get updated snapshotInfo and pass the same to OMSnapshotPurgeResponse [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #7045:
URL: https://github.com/apache/ozone/pull/7045#discussion_r1717322639
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java:
##########
@@ -287,18 +219,25 @@ private void updateSnapshotChainAndCache(
metadataManager.getSnapshotInfoTable().addCacheEntry(
new CacheKey<>(nextPathSnapInfo.getTableKey()),
CacheValue.get(trxnLogIndex, nextPathSnapInfo));
- updatedPathPreviousAndGlobalSnapshots
- .put(nextPathSnapInfo.getTableKey(), nextPathSnapInfo);
} else if (nextGlobalSnapInfo != null) {
nextGlobalSnapInfo.setGlobalPreviousSnapshotId(
snapInfo.getGlobalPreviousSnapshotId());
metadataManager.getSnapshotInfoTable().addCacheEntry(
new CacheKey<>(nextGlobalSnapInfo.getTableKey()),
CacheValue.get(trxnLogIndex, nextGlobalSnapInfo));
- updatedPathPreviousAndGlobalSnapshots
- .put(nextGlobalSnapInfo.getTableKey(), nextGlobalSnapInfo);
}
snapshotChainManager.deleteSnapshot(snapInfo);
}
+
+ private SnapshotInfo getUpdatedSnapshotInfo(String snapshotTableKey, OMMetadataManager omMetadataManager)
+ throws IOException {
+ if (updatedSnapshotInfos.containsKey(snapshotTableKey)) {
+ return updatedSnapshotInfos.get(snapshotTableKey);
+ } else {
+ SnapshotInfo snapshotInfo = omMetadataManager.getSnapshotInfoTable().get(snapshotTableKey);
+ updatedSnapshotInfos.put(snapshotTableKey, snapshotInfo);
+ return snapshotInfo;
+ }
Review Comment:
I didn't use the lambda function because `IOException` needed to be caught and converted to `RunTimeExcpetion`. And then this `RunTimeException` needs proper handling in `validateAndUpdateCache`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9198. Maintain local cache in OMSnapshotPurgeRequest to get updated snapshotInfo and pass the same to OMSnapshotPurgeResponse [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #7045:
URL: https://github.com/apache/ozone/pull/7045#discussion_r1722315937
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java:
##########
@@ -223,18 +219,25 @@ private void updateSnapshotChainAndCache(
metadataManager.getSnapshotInfoTable().addCacheEntry(
new CacheKey<>(nextPathSnapInfo.getTableKey()),
CacheValue.get(trxnLogIndex, nextPathSnapInfo));
- updatedPathPreviousAndGlobalSnapshots
- .put(nextPathSnapInfo.getTableKey(), nextPathSnapInfo);
} else if (nextGlobalSnapInfo != null) {
Review Comment:
It is the same object.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java:
##########
@@ -223,18 +219,25 @@ private void updateSnapshotChainAndCache(
metadataManager.getSnapshotInfoTable().addCacheEntry(
new CacheKey<>(nextPathSnapInfo.getTableKey()),
CacheValue.get(trxnLogIndex, nextPathSnapInfo));
- updatedPathPreviousAndGlobalSnapshots
- .put(nextPathSnapInfo.getTableKey(), nextPathSnapInfo);
} else if (nextGlobalSnapInfo != null) {
Review Comment:
There is no need of this if else if now. We can just directly update :
```
if (nextGlobalSnapInfo != null) {
nextGlobalSnapInfo.setGlobalPreviousSnapshotId(
snapInfo.getGlobalPreviousSnapshotId());
metadataManager.getSnapshotInfoTable().addCacheEntry(
new CacheKey<>(nextGlobalSnapInfo.getTableKey()),
CacheValue.get(trxnLogIndex, nextGlobalSnapInfo));
}
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9198. Maintain local cache in OMSnapshotPurgeRequest to get updated snapshotInfo and pass the same to OMSnapshotPurgeResponse [ozone]
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on code in PR #7045:
URL: https://github.com/apache/ozone/pull/7045#discussion_r1731794340
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java:
##########
@@ -145,7 +145,6 @@ private void updateSnapshotInfoAndCache(SnapshotInfo snapInfo,
// Update table cache first
omMetadataManager.getSnapshotInfoTable().addCacheEntry(new CacheKey<>(snapInfo.getTableKey()),
CacheValue.get(trxnLogIndex, snapInfo));
- updatedSnapInfos.put(snapInfo.getTableKey(), snapInfo);
Review Comment:
We need to add this as well. Because `updateSnapshotInfoAndCache` is being called by
https://github.com/apache/ozone/blob/ab343da40d1dedb7a71a47946ce4eda79015a0dd/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java#L108-L112
This `nextSnapshot` should also be added to `updatedSnapshotInfos` because we are not using `getUpdatedSnapshotInfo` when getting this snapshot so it won't be added to the cache.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9198. Maintain local cache in OMSnapshotPurgeRequest to get updated snapshotInfo and pass the same to OMSnapshotPurgeResponse [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #7045:
URL: https://github.com/apache/ozone/pull/7045#discussion_r1731845112
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java:
##########
@@ -145,7 +145,6 @@ private void updateSnapshotInfoAndCache(SnapshotInfo snapInfo,
// Update table cache first
omMetadataManager.getSnapshotInfoTable().addCacheEntry(new CacheKey<>(snapInfo.getTableKey()),
CacheValue.get(trxnLogIndex, snapInfo));
- updatedSnapInfos.put(snapInfo.getTableKey(), snapInfo);
Review Comment:
I'll also create a follow-up task to add tests to cover this.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9198. Maintain local cache in OMSnapshotPurgeRequest to get updated snapshotInfo and pass the same to OMSnapshotPurgeResponse [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 merged PR #7045:
URL: https://github.com/apache/ozone/pull/7045
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9198. Maintain local cache in OMSnapshotPurgeRequest to get updated snapshotInfo and pass the same to OMSnapshotPurgeResponse [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #7045:
URL: https://github.com/apache/ozone/pull/7045#discussion_r1731835884
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java:
##########
@@ -145,7 +145,6 @@ private void updateSnapshotInfoAndCache(SnapshotInfo snapInfo,
// Update table cache first
omMetadataManager.getSnapshotInfoTable().addCacheEntry(new CacheKey<>(snapInfo.getTableKey()),
CacheValue.get(trxnLogIndex, snapInfo));
- updatedSnapInfos.put(snapInfo.getTableKey(), snapInfo);
Review Comment:
Good catch. Thanks for the catch. Updated.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9198. Maintain local cache in OMSnapshotPurgeRequest to get updated snapshotInfo and pass the same to OMSnapshotPurgeResponse [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on PR #7045:
URL: https://github.com/apache/ozone/pull/7045#issuecomment-2274084649
Pending on #7018.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9198. Maintain local cache in OMSnapshotPurgeRequest to get updated snapshotInfo and pass the same to OMSnapshotPurgeResponse [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #7045:
URL: https://github.com/apache/ozone/pull/7045#discussion_r1717323558
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java:
##########
@@ -87,77 +88,42 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, TermIn
try {
List<String> snapshotDbKeys = snapshotPurgeRequest
.getSnapshotDBKeysList();
- Map<String, SnapshotInfo> updatedSnapInfos = new HashMap<>();
- Map<String, SnapshotInfo> updatedPathPreviousAndGlobalSnapshots =
- new HashMap<>();
// Each snapshot purge operation does three things:
- // 1. Update the snapshot chain,
- // 2. Update the deep clean flag for the next active snapshot (So that it can be
+ // 1. Update the deep clean flag for the next active snapshot (So that it can be
// deep cleaned by the KeyDeletingService in the next run),
+ // 2. Update the snapshot chain,
// 3. Finally, purge the snapshot.
- // All of these steps have to be performed only when it acquires all the necessary
- // locks (lock on the snapshot to be purged, lock on the next active snapshot, and
- // lock on the next path and global previous snapshots). Ideally, there is no need
- // for locks for snapshot purge and can rely on OMStateMachine because OMStateMachine
- // is going to process each request sequentially.
- //
- // But there is a problem with that. After filtering unnecessary SST files for a snapshot,
- // SstFilteringService updates that snapshot's SstFilter flag. SstFilteringService cannot
- // use SetSnapshotProperty API because it runs on each OM independently and One OM does
- // not know if the snapshot has been filtered on the other OM in HA environment.
- //
- // If locks are not taken snapshot purge and SstFilteringService will cause a race condition
- // and override one's update with another.
+ // There is no need to take lock for snapshot purge as of now. We can simply rely on OMStateMachine
+ // because it executes transaction sequentially.
for (String snapTableKey : snapshotDbKeys) {
- // To acquire all the locks, a set is maintained which is keyed by snapshotTableKey.
- // snapshotTableKey is nothing but /volumeName/bucketName/snapshotName.
- // Once all the locks are acquired, it performs the three steps mentioned above and
- // release all the locks after that.
- Set<Triple<String, String, String>> lockSet = new HashSet<>(4, 1);
Review Comment:
Please refer to HDDS-11137/#7018.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9198. Maintain local cache in OMSnapshotPurgeRequest to get updated snapshotInfo and pass the same to OMSnapshotPurgeResponse [ozone]
Posted by "swamirishi (via GitHub)" <gi...@apache.org>.
swamirishi commented on code in PR #7045:
URL: https://github.com/apache/ozone/pull/7045#discussion_r1717651407
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java:
##########
@@ -287,18 +219,25 @@ private void updateSnapshotChainAndCache(
metadataManager.getSnapshotInfoTable().addCacheEntry(
new CacheKey<>(nextPathSnapInfo.getTableKey()),
CacheValue.get(trxnLogIndex, nextPathSnapInfo));
- updatedPathPreviousAndGlobalSnapshots
- .put(nextPathSnapInfo.getTableKey(), nextPathSnapInfo);
} else if (nextGlobalSnapInfo != null) {
nextGlobalSnapInfo.setGlobalPreviousSnapshotId(
snapInfo.getGlobalPreviousSnapshotId());
metadataManager.getSnapshotInfoTable().addCacheEntry(
new CacheKey<>(nextGlobalSnapInfo.getTableKey()),
CacheValue.get(trxnLogIndex, nextGlobalSnapInfo));
- updatedPathPreviousAndGlobalSnapshots
- .put(nextGlobalSnapInfo.getTableKey(), nextGlobalSnapInfo);
}
snapshotChainManager.deleteSnapshot(snapInfo);
}
+
+ private SnapshotInfo getUpdatedSnapshotInfo(String snapshotTableKey, OMMetadataManager omMetadataManager)
+ throws IOException {
+ if (updatedSnapshotInfos.containsKey(snapshotTableKey)) {
+ return updatedSnapshotInfos.get(snapshotTableKey);
+ } else {
+ SnapshotInfo snapshotInfo = omMetadataManager.getSnapshotInfoTable().get(snapshotTableKey);
+ updatedSnapshotInfos.put(snapshotTableKey, snapshotInfo);
+ return snapshotInfo;
+ }
Review Comment:
```suggestion
try {
updatedSnapInfos.computeIfAbsent(snapshotTableKey, k -> {
try {
return omMetadataManager.getSnapshotInfoTable().get(k);
} catch (IOException e) {
throw new UncheckedIOException(e);
}
});
} catch (UncheckedIOException e) {
throw e.getCause();
}
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-9198. Maintain local cache in OMSnapshotPurgeRequest to get updated snapshotInfo and pass the same to OMSnapshotPurgeResponse [ozone]
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #7045:
URL: https://github.com/apache/ozone/pull/7045#discussion_r1719123454
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java:
##########
@@ -287,18 +219,25 @@ private void updateSnapshotChainAndCache(
metadataManager.getSnapshotInfoTable().addCacheEntry(
new CacheKey<>(nextPathSnapInfo.getTableKey()),
CacheValue.get(trxnLogIndex, nextPathSnapInfo));
- updatedPathPreviousAndGlobalSnapshots
- .put(nextPathSnapInfo.getTableKey(), nextPathSnapInfo);
} else if (nextGlobalSnapInfo != null) {
nextGlobalSnapInfo.setGlobalPreviousSnapshotId(
snapInfo.getGlobalPreviousSnapshotId());
metadataManager.getSnapshotInfoTable().addCacheEntry(
new CacheKey<>(nextGlobalSnapInfo.getTableKey()),
CacheValue.get(trxnLogIndex, nextGlobalSnapInfo));
- updatedPathPreviousAndGlobalSnapshots
- .put(nextGlobalSnapInfo.getTableKey(), nextGlobalSnapInfo);
}
snapshotChainManager.deleteSnapshot(snapInfo);
}
+
+ private SnapshotInfo getUpdatedSnapshotInfo(String snapshotTableKey, OMMetadataManager omMetadataManager)
+ throws IOException {
+ if (updatedSnapshotInfos.containsKey(snapshotTableKey)) {
+ return updatedSnapshotInfos.get(snapshotTableKey);
+ } else {
+ SnapshotInfo snapshotInfo = omMetadataManager.getSnapshotInfoTable().get(snapshotTableKey);
+ updatedSnapshotInfos.put(snapshotTableKey, snapshotInfo);
+ return snapshotInfo;
+ }
Review Comment:
I don't think it matters in this case 1). It is in memory map 2). I doubt the key space is going to be more than 100.
Still, I changed the code to do one look-up to address your concern.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org