You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/12/14 02:44:35 UTC
[GitHub] [geode] jchen21 opened a new pull request #7199: GEODE-9770: Retry creating region when cache is closed
jchen21 opened a new pull request #7199:
URL: https://github.com/apache/geode/pull/7199
<!-- 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.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 merged pull request #7199: GEODE-9770: Retry creating region when cache is closed
Posted by GitBox <gi...@apache.org>.
jchen21 merged pull request #7199:
URL: https://github.com/apache/geode/pull/7199
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade commented on a change in pull request #7199: GEODE-9770: Retry creating region when cache is closed
Posted by GitBox <gi...@apache.org>.
agingade commented on a change in pull request #7199:
URL: https://github.com/apache/geode/pull/7199#discussion_r769716240
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/persistence/PersistentRecoveryOrderDUnitTest.java
##########
@@ -1327,8 +1327,19 @@ public void testRecoverAfterConflict() {
vm0.invoke(() -> {
// This should work now
- createReplicateRegion(regionName, getDiskDirs(getVMId()));
- updateEntry("A", "C");
+ try {
+ createReplicateRegion(regionName, getDiskDirs(getVMId()));
+ updateEntry("A", "C");
+ } catch (CacheClosedException cacheClosedException) {
Review comment:
isClosed() checks the isClosing flag; which gets set whiel close is in progress.
Checking for null is not a bad idea; to determine the cache is completely closed.
Other option could be using CacheLifecycleListener...Ex: ModifyColocationIntegrationTest
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade commented on a change in pull request #7199: GEODE-9770: Retry creating region when cache is closed
Posted by GitBox <gi...@apache.org>.
agingade commented on a change in pull request #7199:
URL: https://github.com/apache/geode/pull/7199#discussion_r768883138
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/persistence/PersistentRecoveryOrderDUnitTest.java
##########
@@ -1327,8 +1327,19 @@ public void testRecoverAfterConflict() {
vm0.invoke(() -> {
// This should work now
- createReplicateRegion(regionName, getDiskDirs(getVMId()));
- updateEntry("A", "C");
+ try {
+ createReplicateRegion(regionName, getDiskDirs(getVMId()));
+ updateEntry("A", "C");
+ } catch (CacheClosedException cacheClosedException) {
Review comment:
If cache gets closed whenever "ConflictingPersistentDataExcpetion" is thrown; will it be a good option to wait for cache to be closed before recreating the replicated-region?
That way it becomes easier to understand the flow/expectation at different blocks of the test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade commented on a change in pull request #7199: GEODE-9770: Retry creating region when cache is closed
Posted by GitBox <gi...@apache.org>.
agingade commented on a change in pull request #7199:
URL: https://github.com/apache/geode/pull/7199#discussion_r769730154
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/persistence/PersistentRecoveryOrderDUnitTest.java
##########
@@ -1326,6 +1326,10 @@ public void testRecoverAfterConflict() {
vm1.invoke(() -> getCache().close());
vm0.invoke(() -> {
+ await().untilAsserted(
+ () -> assertThat(
+ InternalDistributedSystem.getAnyInstance().getCancelCriterion().cancelInProgress())
Review comment:
Looking at the code related to cancelInProgress(); it looks like it may also be returning early, by checking isClosing(). I may be missing something...Can you please point me to the path where it shows/checks cacelInProgress is returns null after cache is completely closed...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7199: GEODE-9770: Retry creating region when cache is closed
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #7199:
URL: https://github.com/apache/geode/pull/7199#discussion_r770067321
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/persistence/PersistentRecoveryOrderDUnitTest.java
##########
@@ -1320,6 +1320,7 @@ public void testRecoverAfterConflict() {
createReplicateRegion(regionName, getDiskDirs(getVMId()));
});
assertThat(thrown).isInstanceOf(ConflictingPersistentDataException.class);
+ InternalDistributedSystem.getAnyInstance().getCache().close();
Review comment:
Can you change this to: getCache().close() like other invokes in this test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #7199: GEODE-9770: Retry creating region when cache is closed
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #7199:
URL: https://github.com/apache/geode/pull/7199#discussion_r769871551
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/persistence/PersistentRecoveryOrderDUnitTest.java
##########
@@ -1326,6 +1326,10 @@ public void testRecoverAfterConflict() {
vm1.invoke(() -> getCache().close());
vm0.invoke(() -> {
+ await().untilAsserted(
Review comment:
It might be better to just change this test to test the more normal use case of restarting the member that had the ConflictingPersistentDataException. I think you could do that in this test by closing the cache in vm0 when you catch ConflictingPersistentDataException. Then the test would close vm1 and then when vm0 calls createReplicationRegion it will be with a new cache. I don't think we need to test that a member that saw this exception can later on create the region using the same cache.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on pull request #7199: GEODE-9770: Retry creating region when cache is closed
Posted by GitBox <gi...@apache.org>.
jchen21 commented on pull request #7199:
URL: https://github.com/apache/geode/pull/7199#issuecomment-995166768
Removed the `cancelInProgress` assertion. And explicitly close the cache in the main thread, after seeing `ConflictingPersistentDataException`. So that before recreating the region, the old cache is closed. It will recreate the region in a new cache.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade commented on a change in pull request #7199: GEODE-9770: Retry creating region when cache is closed
Posted by GitBox <gi...@apache.org>.
agingade commented on a change in pull request #7199:
URL: https://github.com/apache/geode/pull/7199#discussion_r770904918
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/persistence/PersistentRecoveryOrderDUnitTest.java
##########
@@ -1320,6 +1320,7 @@ public void testRecoverAfterConflict() {
createReplicateRegion(regionName, getDiskDirs(getVMId()));
});
assertThat(thrown).isInstanceOf(ConflictingPersistentDataException.class);
+ InternalDistributedSystem.getAnyInstance().getCache().close();
Review comment:
Getting the cache using InternalDistributedSystem will work; but still looks some indirect way of closing the cache....
Can this work:
Get the cache reference; try creating the region and then call close on the cache reference.
vm0.invoke(() -> {
cache = getCache();
try (IgnoredException ie = addIgnoredException(ConflictingPersistentDataException.class)) {
// this should cause a conflict
Throwable thrown = catchThrowable(() -> {
createReplicateRegion(regionName, getDiskDirs(getVMId()));
});
assertThat(thrown).isInstanceOf(ConflictingPersistentDataException.class);
}
cache.close();
});
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #7199: GEODE-9770: Retry creating region when cache is closed
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #7199:
URL: https://github.com/apache/geode/pull/7199#discussion_r769052845
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/persistence/PersistentRecoveryOrderDUnitTest.java
##########
@@ -1327,8 +1327,19 @@ public void testRecoverAfterConflict() {
vm0.invoke(() -> {
// This should work now
- createReplicateRegion(regionName, getDiskDirs(getVMId()));
- updateEntry("A", "C");
+ try {
+ createReplicateRegion(regionName, getDiskDirs(getVMId()));
+ updateEntry("A", "C");
+ } catch (CacheClosedException cacheClosedException) {
Review comment:
The question is how we can tell the cache is already closed or is in the process of closing, before recreating the region. There is some subtle difference. `GemFireCacheImpl.isClosed()` returns `true`, even when the bug occurs. So we can't depend on the flag `GemFireCacheImpl.isClosed()`. I have also tried to check if the cache is `null`. Not a good idea either. I probably have to do something similar to `cancelInProgress`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jchen21 commented on a change in pull request #7199: GEODE-9770: Retry creating region when cache is closed
Posted by GitBox <gi...@apache.org>.
jchen21 commented on a change in pull request #7199:
URL: https://github.com/apache/geode/pull/7199#discussion_r770107472
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/persistence/PersistentRecoveryOrderDUnitTest.java
##########
@@ -1320,6 +1320,7 @@ public void testRecoverAfterConflict() {
createReplicateRegion(regionName, getDiskDirs(getVMId()));
});
assertThat(thrown).isInstanceOf(ConflictingPersistentDataException.class);
+ InternalDistributedSystem.getAnyInstance().getCache().close();
Review comment:
There is some difference here.
`getCache()` will create a new cache. Because `isClosing` is `true` in the old cache, due to `ConflictingPersistentDataException`. In JUnit4CacheTestCase, there is the code:
```
if (cache == null || cache.isClosed()) {
cache = null;
createCache(client, factory);
}
```
While `InternalDistributedSystem.getAnyInstance().getCache()` returns the old cache. We want to explicitly close the old cache. So we use `InternalDistributedSystem.getAnyInstance().getCache()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] gesterzhou commented on a change in pull request #7199: GEODE-9770: Retry creating region when cache is closed
Posted by GitBox <gi...@apache.org>.
gesterzhou commented on a change in pull request #7199:
URL: https://github.com/apache/geode/pull/7199#discussion_r769952943
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/persistence/PersistentRecoveryOrderDUnitTest.java
##########
@@ -1326,6 +1326,10 @@ public void testRecoverAfterConflict() {
vm1.invoke(() -> getCache().close());
vm0.invoke(() -> {
+ await().untilAsserted(
+ () -> assertThat(
+ InternalDistributedSystem.getAnyInstance().getCancelCriterion().cancelInProgress())
Review comment:
I think the purpose of the fix is to introduce some wait until vm0 completely closed the cache. Using cancelInProgress() is not easy to understand.
I suggest to use:
await().untilAsserted(()->assertThat(cache == null || cache.isClosed()));
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org