Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HDDS-10103. Simplified snapshotCache with just one ConcurrentHashMap. #5986

Merged
merged 2 commits into from
Jan 11, 2024

Conversation

hemantk-12
Copy link
Contributor

@hemantk-12 hemantk-12 commented Jan 11, 2024

What changes were proposed in this pull request?

Currently we use two different Concurrent collections, ConcurrentMap to keep entries in the cache and ConcurrentSet to keep track for the entries which can be closed. Usage of these two are causing deadlock and race condition problem.

In this change, usage of ConcurrentSet to keep track of closable entries is removed. Now we are iterating over the ConcurrentMap itself and checking if entry's reference size has reached zero and can be closed. If it has, we close the RocksDB object and remove the entry from the SnapshotCache.

What is the link to the Apache JIRA

HDDS-10103

How was this patch tested?

Existing unit and integration tests.

…dingEvictionList and ReferenceCountedCallback.
Copy link
Contributor

@swamirishi swamirishi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch @hemantk-12. The change overal looks very simple everything working under compute method of ConcurrentHashMap. We can look into optimizing the cleanup method to fetch the instances only with 0 references. This would be critical if the cache size is set to be high.

@hemantk-12 hemantk-12 marked this pull request as ready for review January 11, 2024 19:34
Copy link
Contributor

@smengcl smengcl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @hemantk-12 . lgtm. fair trade-off for now

@hemantk-12
Copy link
Contributor Author

hemantk-12 commented Jan 11, 2024

Thanks for the patch @hemantk-12. The changes overal looks very simple everything working under compute method of ConcurrentHashMap. We can look into optimizing the cleanup method to fetch the instances only with 0 references. This would be critical if the cache size is set to be high.

Similar point was raised by Siyao in #5968 (comment). It could be a follow up task.

Copy link
Member

@aswinshakil aswinshakil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM+1. Thanks for the patch @hemantk-12 , just one comment inline. I'll close #5934 in accordance with this PR.

@swamirishi swamirishi merged commit a8341d7 into apache:master Jan 11, 2024
34 checks passed
@hemantk-12
Copy link
Contributor Author

Thanks @aswinshakil, @swamirishi and @smengcl for the review.

@hemantk-12
Copy link
Contributor Author

Follow up task HDDS-10156 to optimize the clean up.

jojochuang pushed a commit to jojochuang/ozone that referenced this pull request Feb 1, 2024
…rentHashMap. (apache#5986)

(cherry picked from commit a8341d7)
Change-Id: Iee0a0e72c73a1d19645655aebc24f4471a493cec
(cherry picked from commit 57a443b27b12dfa71ba4fcd73b93cde97e86f010)
adoroszlai pushed a commit to adoroszlai/ozone that referenced this pull request Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
snapshot https://issues.apache.org/jira/browse/HDDS-6517
Projects
None yet
4 participants