You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@geode.apache.org by "pivotal-eshu (GitHub)" <gi...@apache.org> on 2018/09/21 00:16:40 UTC

[GitHub] [geode] pivotal-eshu opened pull request #2501: GEODE-5748: Hold a write lock during cleanUpAfterFailedGII

 * Hold write lock when cleanUpAfterFailedGII.
 * Hold read lock when cache operation is performed on the farside if region is not yet initialized.
 * Also hold read lock when transaction is performed on the farside if region not initialized yet.


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

[GitHub] [geode] dschneider-pivotal commented on pull request #2501: GEODE-5748: Hold a write lock during cleanUpAfterFailedGII

Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.

declare locked on this line; make it final; give it its own try/finally

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

[GitHub] [geode] dschneider-pivotal commented on pull request #2501: GEODE-5748: Hold a write lock during cleanUpAfterFailedGII

Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.

It would be better for this code to be added to RegionMapCommitPut instead of AbstractRegionMap.
In the RegionMapCommitPut is should be done following the pattern in that class of a method that takes a Runnable.
For an example see: org.apache.geode.internal.cache.map.AbstractRegionMapPut.runWhileLockedForCacheModification(Runnable)
I think you would want to do this in: AbstractRegionMapPut because it would be the same for both a CommitPut and the non-commit put

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

[GitHub] [geode] dschneider-pivotal commented on pull request #2501: GEODE-5748: Hold a write lock during cleanUpAfterFailedGII

Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.

The order should be like this:
A
B
try/finally
B
A

You currently have:
A
B
try/finally
A
B

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

[GitHub] [geode] pivotal-eshu commented on issue #2501: GEODE-5748: Hold a write lock during cleanUpAfterFailedGII

Posted by "pivotal-eshu (GitHub)" <gi...@apache.org>.

We do not have a clean way to address the concern that lock and unlock should be invoked in the same place. We may need to refactor to address the issue.

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

[GitHub] [geode] dschneider-pivotal commented on pull request #2501: GEODE-5748: Hold a write lock during cleanUpAfterFailedGII

Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.

make "locked" final

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

[GitHub] [geode] dschneider-pivotal commented on pull request #2501: GEODE-5748: Hold a write lock during cleanUpAfterFailedGII

Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.

declare "locked" on this line instead of line 1345.
The finally block that does the unlock and the old releaseCacheModLock should not be the outer one that it currently is. In the old code we would of called releaseCacheModLock even if we did not call lockForCacheMod (if waitForIndexInit threw an exception). That is wrong.
I think you should move the outer "try {" down to just after your new call of lockWhenRegionIsInitializing.
Also it would be better to have a separate try/finally for your new lock instead of using the finally block of releaseCacheModLock. Currently the order in the finally block is wrong. But if one of these threw an exception then you would not want to do both of them in the finally block. Also doing them separately allows you to refactor them into a method that takes a lambda like "doWhilePreventingInitializationCleanup".

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

[GitHub] [geode] dschneider-pivotal commented on pull request #2501: GEODE-5748: Hold a write lock during cleanUpAfterFailedGII

Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.

I would remove these two new blank lines and then AbstractRegionMapPut will no longer be modified by this pull request

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

[GitHub] [geode] dschneider-pivotal commented on pull request #2501: GEODE-5748: Hold a write lock during cleanUpAfterFailedGII

Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.

a better name might be "failedInitialImageLock". We want to get away from acronyms. And "clear" does not need to be in the name.

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

[GitHub] [geode] agingade commented on pull request #2501: GEODE-5748: Hold a write lock during cleanUpAfterFailedGII

Posted by "agingade (GitHub)" <gi...@apache.org>.

In this case, it doesn't look like we need to have fair lock; as there are only few write lock to be acquired, during failed clear. How about using StoppableReentrantReadWriteLock which takes cancel criteria.

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

[GitHub] [geode] dschneider-pivotal commented on pull request #2501: GEODE-5748: Hold a write lock during cleanUpAfterFailedGII

Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.

these changes should be in AbstractRegionMapPut to be shared with CommitPut

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

[GitHub] [geode] dschneider-pivotal commented on pull request #2501: GEODE-5748: Hold a write lock during cleanUpAfterFailedGII

Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.

see other comments. I know you are adding calls every place we do lockForCacheModification but I do wonder if this one is needed while initializing. Better safe than sorry, but I was wondering if you know if basicUpdateEntryVersion needed to be prevented during cleanup after a failed GII

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

[GitHub] [geode] pivotal-eshu closed pull request #2501: GEODE-5748: Hold a write lock during cleanUpAfterFailedGII

Posted by "pivotal-eshu (GitHub)" <gi...@apache.org>.

[ pull request closed by pivotal-eshu ]

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

[GitHub] [geode] dschneider-pivotal commented on pull request #2501: GEODE-5748: Hold a write lock during cleanUpAfterFailedGII

Posted by "dschneider-pivotal (GitHub)" <gi...@apache.org>.

It would be nice if we could share this code by putting it in AbstractRegionMapPut.
Here is how we could do it. Move the runWhileLockedForCacheModification you have in RegionMapCommitPut to AbstractRegionMapPut. Then change this one to just do the old cacheModLock but in the body do this:
super. runWhileLockedForCacheModification(r);

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