[GitHub] [geode] nabarunnag opened pull request #2776: GEODE-5982: Synchronized access to CacheLoader and CacheWriter

["nabarunnag (GitHub)" <gi...@apache.org>]

	* Using read-write locks to synchronize the getters and setters for CacheLoader and CacheWriter.

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, you check travis-ci 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/2776 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] nabarunnag closed pull request #2776: GEODE-5982: Synchronized access to CacheLoader and CacheWriter

["nabarunnag (GitHub)" <gi...@apache.org>]

[ pull request closed by nabarunnag ]

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

[GitHub] [geode] bschuchardt commented on issue #2776: GEODE-5982: Synchronized access to CacheLoader and CacheWriter

["bschuchardt (GitHub)" <gi...@apache.org>]

It looks like you have a bug to look into
org.apache.geode.cache30.DiskRegionIntegrationTest > testLRUCacheLoader FAILED
    java.lang.IllegalMonitorStateException
        at java.util.concurrent.locks.ReentrantReadWriteLock$Sync.tryRelease(ReentrantReadWriteLock.java:371)
        at java.util.concurrent.locks.AbstractQueuedSynchronizer.release(AbstractQueuedSynchronizer.java:1261)
        at java.util.concurrent.locks.ReentrantReadWriteLock$WriteLock.unlock(ReentrantReadWriteLock.java:1131)
        at org.apache.geode.internal.cache.AbstractRegion.setCacheLoader(AbstractRegion.java:1165)
        at org.apache.geode.cache30.DiskRegionIntegrationTest.testLRUCacheLoader(DiskRegionIntegrationTest.java:551)

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

[GitHub] [geode] jdeppe-pivotal commented on issue #2776: GEODE-5982: Synchronized access to CacheLoader and CacheWriter

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

I don't really know the surrounding context very well, but at face value, I'm not really sure that this change improves anything. For example it seems that 'thread1' could have retrieved the current loader and, before finishing, thread2 changes out the loader and calls `close()` on the old loader before thread1 completes and is done using that old loader.

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

[GitHub] [geode] kirklund commented on pull request #2776: GEODE-5982: Synchronized access to CacheLoader and CacheWriter

["kirklund (GitHub)" <gi...@apache.org>]

Shouldn't these be private? If they are accessed from another class, then a better approach would be either of these:

1) add getters -- doesn't really improve it a lot but at least we aren't reaching into another class to access and use fat locks.

2) make the uses of these locks completely contained within this class. If you really need to inject code from another class to run within a block holding these locks, then try something like:

```
  public static void execute(final Runnable action) {
    // acquire lock
    try {
      action.run();
    } finally {
      // release lock
    }
  }
```

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