You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "smengcl (via GitHub)" <gi...@apache.org> on 2023/04/14 15:07:05 UTC
[GitHub] [ozone] smengcl opened a new pull request, #4568: HDDS-7935. [Snapshot] LRU Cache entries may get evicted/closed during long running processes
smengcl opened a new pull request, #4568:
URL: https://github.com/apache/ozone/pull/4568
## What changes were proposed in this pull request?
EXPERIMENTAL.
This is approach 2 of 2 that might fix the issue that bothers SnapDiff, where the current `LoadingCache` behaves like a simple LRU cache. We had no control over when an `OmSnapshot` instance can be evicted and closed, which can cause the the snapshot DB instance to be **closed prematurely** while SnapDiff is still running in the background, crashing the OM.
For approach 1 that implements a custom `SnapshotCache` and the whole modified-LRU logic from scratch, see #4567 .
This approach 2 replaces the hard-limit (`.maximumSize()`) with `.weakValues()`. This allows JVM garbage collector to collect the value when they are no longer strongly referenced, for instance from SnapDiff or Hadoop FS API read operations.
The `ozone.om.snapshot.cache.max.size` effectively becomes a soft limit (the same as approach 1), with warning printed in `checkForSnapshot()` when cache size exceeds the soft limit.
This is not fully tested yet. This is much cleaner than approach 1 if this works as expected. In the worst case, we fall back to approach 1.
cc @GeorgeJahad
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-7935
## How was this patch tested?
- All existing test should pass.
- Pending SnapDiff test additions that intentionally exceeds the cache limit.
- Possibly new test cases that triggers GC while SnapDiff is still running to see if it can still finish without crashing OM.
--
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
[GitHub] [ozone] hemantk-12 commented on pull request #4568: HDDS-7935. [Snapshot] LRU Cache entries may get evicted/closed during long running processes
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on PR #4568:
URL: https://github.com/apache/ozone/pull/4568#issuecomment-1532111352
> > One question though, do you know if it is possible to set both softValues() and maximumSize() on the cache? Would that give us the desired behaviour without reference counting?
>
> @GeorgeJahad AFAIU, unfortunately as soon as `maximumSize(limit)` is added, `limit` becomes a hard limit. It will start invalidating cache entries when the `limit` would be exceeded thus we would have the same issue as before.
That's my understanding too.
--
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
[GitHub] [ozone] smengcl commented on pull request #4568: HDDS-7935. [Snapshot] LRU Cache entries may get evicted/closed during long running processes
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #4568:
URL: https://github.com/apache/ozone/pull/4568#issuecomment-1532060907
> One question though, do you know if it is possible to set both softValues() and maximumSize() on the cache? Would that give us the desired behaviour without reference counting?
@GeorgeJahad AFAIU, unfortunately as soon as `maximumSize(limit)` is added, `limit` becomes a hard limit. It will start invalidating cache entries when the `limit` is exceeded thus would have the same issue as before.
--
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
[GitHub] [ozone] smengcl commented on a diff in pull request #4568: HDDS-7935. [Snapshot] LRU Cache entries may get evicted/closed during long running processes
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4568:
URL: https://github.com/apache/ozone/pull/4568#discussion_r1179494241
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -558,10 +584,14 @@ public SnapshotDiffResponse getSnapshotDiffReport(final String volume,
final String fsKey = SnapshotInfo.getTableKey(volume, bucket, fromSnapshot);
final String tsKey = SnapshotInfo.getTableKey(volume, bucket, toSnapshot);
try {
+ // Block SnapDiff if either one of the snapshot is not active.
+ checkSnapshotActive(fsKey);
Review Comment:
nice
--
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
[GitHub] [ozone] smengcl commented on a diff in pull request #4568: HDDS-7935. [Snapshot] LRU Cache entries may get evicted/closed during long running processes
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4568:
URL: https://github.com/apache/ozone/pull/4568#discussion_r1179468811
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -292,7 +309,7 @@ public OmSnapshot load(@Nonnull String snapshotTableKey)
snapshotMetadataManager = new OmMetadataManagerImpl(conf,
snapshotInfo.getCheckpointDirName(), isSnapshotInCache);
} catch (IOException e) {
- LOG.error("Failed to retrieve snapshot: {}, {}", snapshotTableKey, e);
+ LOG.error("Failed to retrieve snapshot: {}", snapshotTableKey, e);
Review Comment:
Removed `e` from param list.
--
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
[GitHub] [ozone] smengcl commented on a diff in pull request #4568: HDDS-7935. [Snapshot] LRU Cache entries may get evicted/closed during long running processes
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4568:
URL: https://github.com/apache/ozone/pull/4568#discussion_r1179468312
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -241,6 +253,30 @@ public OmSnapshotManager(OzoneManager ozoneManager) {
this.snapshotDiffCleanupService.start();
}
+ /**
+ * Throws OMException FILE_NOT_FOUND if snapshot is not in active status.
+ * @param snapshotTableKey snapshot table key
+ */
+ public void checkSnapshotActive(String snapshotTableKey) throws IOException {
+ checkSnapshotActive(getSnapshotInfo(snapshotTableKey));
+ }
+
+ private void checkSnapshotActive(SnapshotInfo snapInfo) throws OMException {
+
+ if (!snapInfo.getSnapshotStatus().equals(SnapshotStatus.SNAPSHOT_ACTIVE)) {
Review Comment:
good catch
--
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
[GitHub] [ozone] smengcl commented on a diff in pull request #4568: HDDS-7935. [Snapshot] LRU Cache entries may get evicted/closed during long running processes
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4568:
URL: https://github.com/apache/ozone/pull/4568#discussion_r1179473455
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -241,6 +253,30 @@ public OmSnapshotManager(OzoneManager ozoneManager) {
this.snapshotDiffCleanupService.start();
}
+ /**
+ * Throws OMException FILE_NOT_FOUND if snapshot is not in active status.
+ * @param snapshotTableKey snapshot table key
+ */
+ public void checkSnapshotActive(String snapshotTableKey) throws IOException {
Review Comment:
fair point. moved to `SnapshotUtils`.
--
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
[GitHub] [ozone] GeorgeJahad commented on pull request #4568: HDDS-7935. [Snapshot] LRU Cache entries may get evicted/closed during long running processes
Posted by "GeorgeJahad (via GitHub)" <gi...@apache.org>.
GeorgeJahad commented on PR #4568:
URL: https://github.com/apache/ozone/pull/4568#issuecomment-1531868414
One question though, do you know if it is possible to set both softValues() and maximumSize() on the cache? Would that give us the desired behaviour without reference counting?
--
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
[GitHub] [ozone] hemantk-12 commented on a diff in pull request #4568: HDDS-7935. [Snapshot] LRU Cache entries may get evicted/closed during long running processes
Posted by "hemantk-12 (via GitHub)" <gi...@apache.org>.
hemantk-12 commented on code in PR #4568:
URL: https://github.com/apache/ozone/pull/4568#discussion_r1178722627
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -241,6 +253,30 @@ public OmSnapshotManager(OzoneManager ozoneManager) {
this.snapshotDiffCleanupService.start();
}
+ /**
+ * Throws OMException FILE_NOT_FOUND if snapshot is not in active status.
+ * @param snapshotTableKey snapshot table key
+ */
+ public void checkSnapshotActive(String snapshotTableKey) throws IOException {
+ checkSnapshotActive(getSnapshotInfo(snapshotTableKey));
+ }
+
+ private void checkSnapshotActive(SnapshotInfo snapInfo) throws OMException {
+
+ if (!snapInfo.getSnapshotStatus().equals(SnapshotStatus.SNAPSHOT_ACTIVE)) {
Review Comment:
```suggestion
if (snapshotInfo.getSnapshotStatus() != SnapshotStatus.SNAPSHOT_ACTIVE) {
```
nit to avoid null check.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -558,10 +584,14 @@ public SnapshotDiffResponse getSnapshotDiffReport(final String volume,
final String fsKey = SnapshotInfo.getTableKey(volume, bucket, fromSnapshot);
final String tsKey = SnapshotInfo.getTableKey(volume, bucket, toSnapshot);
try {
+ // Block SnapDiff if either one of the snapshot is not active.
+ checkSnapshotActive(fsKey);
Review Comment:
We do this check in `verifySnapshotInfoForSnapDiff` at line number 574. May be you can change `verifySnapshotInfoForSnapDiff`.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -241,6 +253,30 @@ public OmSnapshotManager(OzoneManager ozoneManager) {
this.snapshotDiffCleanupService.start();
}
+ /**
+ * Throws OMException FILE_NOT_FOUND if snapshot is not in active status.
+ * @param snapshotTableKey snapshot table key
+ */
+ public void checkSnapshotActive(String snapshotTableKey) throws IOException {
Review Comment:
I think it would be better to move this to [SnapshotUtils](https://github.com/apache/ozone/blob/475963125b4a03a79a25519fe91fab4e1d6b36dc/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotUtils.java#L36). I don't like the way it is used in `SnapshotDiffManager`.
```
ozoneManager.getOmSnapshotManager().checkSnapshotActive(fsKey);
ozoneManager.getOmSnapshotManager().checkSnapshotActive(tsKey);
```
I just realized that we have two Utils classes SnapshotUtils (created by me :facepalm:) and OmSnapshotUtils. We can merge them to one as a separate task.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java:
##########
@@ -260,6 +260,16 @@ public void close() throws IOException {
omMetadataManager.getStore().close();
}
+ @Override
+ protected void finalize() throws Throwable {
Review Comment:
It is not generally [advice to override finalize](https://softwareengineering.stackexchange.com/questions/288715/is-overriding-object-finalize-really-bad). I see you did it to close GCed objects but I don't think it is needed. RemovalListener will run on `Collected: Entry was removed automatically because its key or value was garbage-collected.`
Point#3 in https://www.educative.io/answers/how-to-use-eviction-listeners-and-statistics-in-guava-cache
https://guava.dev/releases/19.0/api/docs/com/google/common/cache/RemovalCause.html#COLLECTED
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshotManager.java:
##########
@@ -292,7 +309,7 @@ public OmSnapshot load(@Nonnull String snapshotTableKey)
snapshotMetadataManager = new OmMetadataManagerImpl(conf,
snapshotInfo.getCheckpointDirName(), isSnapshotInCache);
} catch (IOException e) {
- LOG.error("Failed to retrieve snapshot: {}, {}", snapshotTableKey, e);
+ LOG.error("Failed to retrieve snapshot: {}", snapshotTableKey, e);
Review Comment:
Curious if we are doing double logging. Won't it be thrown as ExecutionException.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotFileSystem.java:
##########
@@ -411,15 +411,19 @@ public void testBlockSnapshotFSAccessAfterDeletion() throws Exception {
deleteSnapshot(snapshotName);
// Can't access keys in snapshot anymore with FS API. Should throw exception
- final String errorMsg = "no longer active";
- LambdaTestUtils.intercept(OMException.class, errorMsg,
+ final String errorMsg1 = "no longer active";
+ LambdaTestUtils.intercept(FileNotFoundException.class, errorMsg1,
Review Comment:
+1 on checking exception.
--
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
[GitHub] [ozone] smengcl commented on pull request #4568: HDDS-7935. [Snapshot] LRU Cache entries may get evicted/closed during long running processes
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #4568:
URL: https://github.com/apache/ozone/pull/4568#issuecomment-1526046379
@GeorgeJahad Please kindly take a look at this. We plan to fix the early-close issue at hand with `softValues()` first, then do the custom reference counting ( i.e. #4567 ).
--
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
[GitHub] [ozone] smengcl merged pull request #4568: HDDS-7935. [Snapshot] LRU Cache entries may get evicted/closed during long running processes
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl merged PR #4568:
URL: https://github.com/apache/ozone/pull/4568
--
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
[GitHub] [ozone] GeorgeJahad commented on pull request #4568: HDDS-7935. [Snapshot] LRU Cache entries may get evicted/closed during long running processes
Posted by "GeorgeJahad (via GitHub)" <gi...@apache.org>.
GeorgeJahad commented on PR #4568:
URL: https://github.com/apache/ozone/pull/4568#issuecomment-1531863559
lgtm. thanks for taking this off my hands!
--
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
[GitHub] [ozone] smengcl commented on a diff in pull request #4568: HDDS-7935. [Snapshot] LRU Cache entries may get evicted/closed during long running processes
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4568:
URL: https://github.com/apache/ozone/pull/4568#discussion_r1179515860
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java:
##########
@@ -260,6 +260,16 @@ public void close() throws IOException {
omMetadataManager.getStore().close();
}
+ @Override
+ protected void finalize() throws Throwable {
Review Comment:
good point. I've tweaked the logic a bit and added logging.
--
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
[GitHub] [ozone] smengcl commented on a diff in pull request #4568: HDDS-7935. [Snapshot] LRU Cache entries may get evicted/closed during long running processes
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #4568:
URL: https://github.com/apache/ozone/pull/4568#discussion_r1179517614
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java:
##########
@@ -260,6 +260,16 @@ public void close() throws IOException {
omMetadataManager.getStore().close();
}
+ @Override
+ protected void finalize() throws Throwable {
Review Comment:
Thanks a lot for the pointers!
--
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