jchen21 commented on a change in pull request #6964:
URL: https://github.com/apache/geode/pull/6964#discussion_r727558560
##########
File path:
geode-core/src/test/java/org/apache/geode/internal/cache/PartitionedRegionTest.java
##########
@@ -443,6 +449,40 @@ public void
generatePRIdShouldNotThrowNumberFormatExceptionIfAnErrorOccursWhileR
.doesNotThrowAnyException();
}
+ @Test
+ public void
registerPartitionedRegionShouldHandleLockServiceDestroyedException()
+ throws ClassNotFoundException {
+ AttributesFactory attributesFactory = new AttributesFactory();
+ attributesFactory.setPartitionAttributes(
+ new
PartitionAttributesFactory().setTotalNumBuckets(1).setRedundantCopies(1)
+ .setLocalMaxMemory(0).create());
+
+ partitionedRegion = new PartitionedRegion("region",
attributesFactory.create(), null, cache,
+ mock(InternalRegionArguments.class), disabledClock(),
ColocationLoggerFactory.create());
+
+ PartitionedRegion spyPartitionedRegion = spy(partitionedRegion);
+
+ InternalDistributedMember imageTarget =
mock(InternalDistributedMember.class);
+ InternalRegionFactory factory = mock(InternalRegionFactory.class);
+ DistributedRegion partitionedRegionRoot = mock(DistributedRegion.class);
+ CacheDistributionAdvisor cda = mock(CacheDistributionAdvisor.class);
+ PartitionedRegion.RegionLock regionLock =
mock(PartitionedRegion.RegionLock.class);
+
+
when(cache.createInternalRegionFactory(RegionShortcut.REPLICATE)).thenReturn(factory);
+
when(factory.create(PR_ROOT_REGION_NAME)).thenReturn(partitionedRegionRoot);
+ doNothing().when(cda).addMembershipListener(any());
+ when(partitionedRegionRoot.getDistributionAdvisor()).thenReturn(cda);
+ doReturn(regionLock).when(spyPartitionedRegion).getRegionLock();
+ doThrow(new LockServiceDestroyedException("Lock Service is destroyed in
test")).when(regionLock)
+ .lock();
+ doThrow(new
DistributedSystemDisconnectedException("test")).when(spyPartitionedRegion)
+ .cleanupFailedInitialization();
+
+ assertThatThrownBy(() -> spyPartitionedRegion.initialize(null,
imageTarget, null))
+ .isInstanceOf(PartitionedRegionException.class)
+ .hasCauseInstanceOf(DistributedSystemDisconnectedException.class);
Review comment:
1. Also need to assert `DistributedSystemDisconnectedException` has the
cause instance of `LockServiceDestroyedException`.
2. Need one more test case to cover the following code, when there is no
`DistributedSystemDisconnectedException`.
```
if (pre == null) {
pre = new PartitionedRegionException(
"Can not create PartitionedRegion (failed to acquire
RegionLock).", lsde);
}
```
3. We also need code coverage for the code changes in
`cleanupFailedInitialization()`.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
##########
@@ -1464,12 +1465,26 @@ private void registerPartitionedRegion(boolean
storesData) {
if (logger.isDebugEnabled()) {
logger.debug("registerPartitionedRegion: unable to obtain lock for
{}", this);
}
- cleanupFailedInitialization();
- throw new PartitionedRegionException(
- "Can not create PartitionedRegion (failed to acquire RegionLock).",
- lsde);
+ PartitionedRegionException pre = null;
+ try {
+ cleanupFailedInitialization();
+ } catch (DistributedSystemDisconnectedException rte) {
Review comment:
Is it possible that `cleanupFailedInitialization()` could throw a
`RuntimeException` other than `DistributedSystemDisconnectedException`? If yes,
the code does not catch it, which might not be the correct behavior.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/PartitionedRegion.java
##########
@@ -1464,12 +1465,26 @@ private void registerPartitionedRegion(boolean
storesData) {
if (logger.isDebugEnabled()) {
logger.debug("registerPartitionedRegion: unable to obtain lock for
{}", this);
}
- cleanupFailedInitialization();
- throw new PartitionedRegionException(
- "Can not create PartitionedRegion (failed to acquire RegionLock).",
- lsde);
+ PartitionedRegionException pre = null;
+ try {
+ cleanupFailedInitialization();
+ } catch (DistributedSystemDisconnectedException rte) {
+ rte.initCause(lsde);
+ pre = new PartitionedRegionException(
+ "Can not create PartitionedRegion (failed to acquire
RegionLock).", rte);
+ }
+ if (pre == null) {
+ pre = new PartitionedRegionException(
+ "Can not create PartitionedRegion (failed to acquire
RegionLock).", lsde);
+ }
+ throw pre;
} catch (IllegalStateException ill) {
- cleanupFailedInitialization();
+ try {
+ cleanupFailedInitialization();
+ } catch (DistributedSystemDisconnectedException rte) {
+ logger.info("IllegalStateException " + ill
Review comment:
Better use `logger.warn()`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
