GEODE-7828: Convert backing store for Redis Hashes and Sets to single regions #4745
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… regions This PR builds on the work originally submitted by Greg Green in apache#404. Also acknowledgements to Galen O'Sullivan for addressing locking issues in that PR. This commit changes the storage model of Redis hashes and sets from one region per each Redis key to a single hash region and single set region. The Redis key is now also the region key and the data is stored in a Map and Set respectively in the region. Currently, the backing values do not implement Delta changes, however this will be a future optimization. This also fixes the inability of Redis keys to contain other characters commonly used, such as colons (':'). - Add `RedisLockService` which manages a lock per key within a single JVM. Locks are held in a WeakHasMap to allow for automatic cleanup (prior PR work, using a pure ConcurrentHashMap, ended up leaking memory since there is no straight-forward way to clean up unused keys/locks). - Add new tests including concurrency tests for hashes and sets Co-authored-by: Greg Green <ggreen@pivotal.io> Co-authored-by: Ray Ingles <ringles@pivotal.io> Co-authored-by: Sarah Abbey <sabbey@pivotal.io> Co-authored-by: John Hutchison <jhutchison@pivotal.io> Co-authored-by: Murtuza Boxwala <mboxwala@pivotal.io> Co-authored-by: Prasath Durairaj <prasathd@pivotal.io> Co-authored-by: Jens Deppe <jdeppe@pivotal.io>
|
Apologies that this PR is so big - it does also include a bunch of formatting changes. The primary changes are mostly reflected in the following classes:
|
|
This pull request introduces 1 alert when merging e7456e1 into ca7ccbc - view on LGTM.com new alerts:
|
|
LGTM. I do have a question around the RedisLockService. This is a pessimistic locking approach. Is there a reason why we chose that over an optimistic locking approach? |
kohlmu-pivotal
approved these changes
Mar 3, 2020
|
The |
nabarunnag
added a commit
to nabarunnag/geode
that referenced
this pull request
Mar 6, 2020
…o single regions (apache#4745)" This reverts commit 2f6bf01.
6 tasks
jdeppe-pivotal
pushed a commit
that referenced
this pull request
Mar 10, 2020
igodwin
pushed a commit
to igodwin/geode
that referenced
this pull request
Mar 11, 2020
…o single regions (apache#4745)" (apache#4780) This reverts commit 2f6bf01.
igodwin
pushed a commit
to igodwin/geode
that referenced
this pull request
Mar 11, 2020
… regions (apache#4781) * Revert "Revert "GEODE-7828: Convert backing store for Redis Hashes and Sets to single regions (apache#4745)" (apache#4780)" This reverts commit f0982cd. * Fix sporadic test failures for concurrent HSetNX
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR builds on the work originally submitted by Greg Green in
#404. Also acknowledgements to Galen
O'Sullivan for addressing locking issues in that PR.
This commit changes the storage model of Redis hashes and sets from one
region per each Redis key to a single hash region and single set region.
The Redis key is now also the region key and the data is stored in a Map
and Set respectively in the region. Currently, the backing values do not
implement Delta changes, however this will be a future optimization.
This also fixes the inability of Redis keys to contain other characters
commonly used, such as colons (':').
RedisLockServicewhich manages a lock per key within a singleJVM. Locks are held in a WeakHasMap to allow for automatic cleanup
(prior PR work, using a pure ConcurrentHashMap, ended up leaking
memory since there is no straight-forward way to clean up unused
keys/locks).
Co-authored-by: Greg Green ggreen@pivotal.io
Co-authored-by: Ray Ingles ringles@pivotal.io
Co-authored-by: Sarah Abbey sabbey@pivotal.io
Co-authored-by: John Hutchison jhutchison@pivotal.io
Co-authored-by: Murtuza Boxwala mboxwala@pivotal.io
Co-authored-by: Prasath Durairaj prasathd@pivotal.io
Co-authored-by: Jens Deppe jdeppe@pivotal.io
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
For all changes:
Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
Has your PR been rebased against the latest commit within the target branch (typically
develop)?Is your initial contribution a single, squashed commit?
Does
gradlew buildrun cleanly?Have you written or updated unit tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.