HDDS-7935. [Snapshot] LRU Cache entries may get evicted/closed during long running processes

[smengcl]

What changes were proposed in this pull request?

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 .softValues(). 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. OmSnapshot#finalize addition should be able to properly close the RocksDB handle.

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.

In a degenerate case where OM JVM heap usage is pushed to the absolute limit, JVM GC could collect OmSnapshot in cache as soon as e.g. a Hadoop FS API read request is served. However, in this case OM performance would tank anyway and the snapshot cache would be the least of its concern. Note: OmSnapshot should not consume much (on-heap) memory by itself. The DB instance could consume a good chunk of off-heap memory which is not JVM's concern.

Under normal conditions, the cache would still be able to serve its purpose.

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.

We are also already overriding finalize() in PipeInputStream and other Managed DB wrappers. So I guess it is fine (for now) to add one more usage here in OmSnapshot.

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.

[hemantk-12]

Thanks for the patch @smengcl

[hemantk-12]

+1 on checking exception.

[smengcl]

@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 ).

[prashantpogde]

thanks for the patch @smengcl. The changes look good to me.

[hemantk-12]

LGTM.

[GeorgeJahad]

lgtm. thanks for taking this off my hands!

[GeorgeJahad]

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?

[smengcl]

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.

[hemantk-12]

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.

Yes, that's my understanding too.