[GitHub] [geode] jdeppe-pivotal opened pull request #4745: GEODE-7828: Convert backing store for Redis Hashes and Sets to single regions

["jdeppe-pivotal (GitHub)" <gi...@apache.org>]

This PR builds on the work originally submitted by Greg Green in
https://github.com/apache/geode/pull/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 <gg...@pivotal.io>
Co-authored-by: Ray Ingles <ri...@pivotal.io>
Co-authored-by: Sarah Abbey <sa...@pivotal.io>
Co-authored-by: John Hutchison <jh...@pivotal.io>
Co-authored-by: Murtuza Boxwala <mb...@pivotal.io>
Co-authored-by: Prasath Durairaj <pr...@pivotal.io>
Co-authored-by: Jens Deppe <jd...@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 build` run 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](http://www.apache.org/legal/resolved.html#category-a)?

### 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.


[ Full content available at: https://github.com/apache/geode/pull/4745 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] lgtm-com[bot] commented on issue #4745: GEODE-7828: Convert backing store for Redis Hashes and Sets to single regions

[lg...@gitbox.apache.org]

This pull request **introduces 1 alert** when merging e7456e1ea089433d2ec5d02f998774a100c8a463 into ca7ccbce73d436005fe027f31ee910ee9beeb769 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-142f1a0ab7663857e5b69d0274f7e083b2375fea)

**new alerts:**

* 1 for Boxed variable is never null

[ Full content available at: https://github.com/apache/geode/pull/4745 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jdeppe-pivotal commented on issue #4745: GEODE-7828: Convert backing store for Redis Hashes and Sets to single regions

["jdeppe-pivotal (GitHub)" <gi...@apache.org>]

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:
- `HashExecutor`
- `SetExecutor`
- `ExecutionHandlerContext`
- `RedisLockService`
- `GeodeRedisServer`

[ Full content available at: https://github.com/apache/geode/pull/4745 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kohlmu-pivotal commented on issue #4745: GEODE-7828: Convert backing store for Redis Hashes and Sets to single regions

["kohlmu-pivotal (GitHub)" <gi...@apache.org>]

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?

[ Full content available at: https://github.com/apache/geode/pull/4745 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org