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/03/18 17:09:54 UTC
[GitHub] [geode] DonalEvans commented on a change in pull request #6132: Rebalance during transaction commit throws RegionDestroyedException
DonalEvans commented on a change in pull request #6132:
URL: https://github.com/apache/geode/pull/6132#discussion_r597076953
##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/TXStateTest.java
##########
@@ -390,4 +391,31 @@ public void firePendingCallbacksSetsChangeAppliedToCacheInEventLocalFilterInfo()
verify(filterInfo1).setChangeAppliedToCache(true);
verify(filterInfo2).setChangeAppliedToCache(true);
}
+
+ @Test
+ public void commitConvertsCommitConflictExceptionToTransactionDataRebalancedExceptionIfCausedByRegionDestroyedExceptionFromReserveAndCheck() {
+ TXState txState = spy(new TXState(txStateProxy, false, disabledClock()));
+ CommitConflictException cce = new CommitConflictException("Conflict caused by cache exception");
+ RegionDestroyedException rde =
+ new RegionDestroyedException("Region was destroyed", "pathToRegion");
+ cce.initCause(rde);
+
+ doThrow(cce).when(txState).reserveAndCheck();
+
+ Throwable caughtException = catchThrowable(txState::commit);
+
+ assertThat(caughtException).isInstanceOf(TransactionDataRebalancedException.class);
+ }
+
+ @Test
+ public void commitThrowsCommitConflictExceptionFromReserveAndCheck() {
+ TXState txState = spy(new TXState(txStateProxy, false, disabledClock()));
+ CommitConflictException cce = new CommitConflictException("Conflict caused by cache exception");
+
+ doThrow(cce).when(txState).reserveAndCheck();
+
+ Throwable caughtException = catchThrowable(txState::commit);
+
+ assertThat(caughtException).isEqualTo(cce);
Review comment:
This can be simplified:
```suggestion
assertThatThrownBy(txState::commit).isEqualTo(cce);
```
##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/TXStateTest.java
##########
@@ -390,4 +391,31 @@ public void firePendingCallbacksSetsChangeAppliedToCacheInEventLocalFilterInfo()
verify(filterInfo1).setChangeAppliedToCache(true);
verify(filterInfo2).setChangeAppliedToCache(true);
}
+
+ @Test
+ public void commitConvertsCommitConflictExceptionToTransactionDataRebalancedExceptionIfCausedByRegionDestroyedExceptionFromReserveAndCheck() {
+ TXState txState = spy(new TXState(txStateProxy, false, disabledClock()));
+ CommitConflictException cce = new CommitConflictException("Conflict caused by cache exception");
+ RegionDestroyedException rde =
+ new RegionDestroyedException("Region was destroyed", "pathToRegion");
+ cce.initCause(rde);
Review comment:
The "caused by" can be set in the constructor of the CCE:
```suggestion
RegionDestroyedException rde =
new RegionDestroyedException("Region was destroyed", "pathToRegion");
CommitConflictException cce = new CommitConflictException("Conflict caused by cache exception", rde);
```
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/TXState.java
##########
@@ -459,16 +459,24 @@ public void commit() throws CommitConflictException {
*/
try {
lockBucketRegions();
+ if (this.locks == null) {
+ reserveAndCheck();
+ }
} catch (PrimaryBucketException pbe) {
// not sure what to do here yet
RuntimeException re = new TransactionDataRebalancedException(
"Transactional data moved, due to rebalancing.");
re.initCause(pbe);
throw re;
- }
-
- if (this.locks == null) {
- reserveAndCheck();
+ } catch (CommitConflictException cce) {
+ if (cce.getCause() instanceof RegionDestroyedException) {
+ RuntimeException re = new TransactionDataRebalancedException(
+ "Transactional data moved, due to rebalancing.");
+ re.initCause(cce);
+ throw re;
+ } else {
+ throw cce;
+ }
}
Review comment:
These blocks can be simplified:
```suggestion
} catch (PrimaryBucketException pbe) {
// not sure what to do here yet
throw new TransactionDataRebalancedException(
"Transactional data moved, due to rebalancing.", pbe);
} catch (CommitConflictException cce) {
if (cce.getCause() instanceof RegionDestroyedException) {
throw new TransactionDataRebalancedException(
"Transactional data moved, due to rebalancing.", cce);
} else {
throw cce;
}
}
```
##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/TXStateTest.java
##########
@@ -390,4 +391,31 @@ public void firePendingCallbacksSetsChangeAppliedToCacheInEventLocalFilterInfo()
verify(filterInfo1).setChangeAppliedToCache(true);
verify(filterInfo2).setChangeAppliedToCache(true);
}
+
+ @Test
+ public void commitConvertsCommitConflictExceptionToTransactionDataRebalancedExceptionIfCausedByRegionDestroyedExceptionFromReserveAndCheck() {
+ TXState txState = spy(new TXState(txStateProxy, false, disabledClock()));
+ CommitConflictException cce = new CommitConflictException("Conflict caused by cache exception");
+ RegionDestroyedException rde =
+ new RegionDestroyedException("Region was destroyed", "pathToRegion");
+ cce.initCause(rde);
+
+ doThrow(cce).when(txState).reserveAndCheck();
+
+ Throwable caughtException = catchThrowable(txState::commit);
+
+ assertThat(caughtException).isInstanceOf(TransactionDataRebalancedException.class);
Review comment:
This can be condensed to one line, and an additional assertion added to confirm that the thrown exception wraps `cce`:
```suggestion
assertThatThrownBy(txState::commit).isInstanceOf(TransactionDataRebalancedException.class)
.hasCause(cce);
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org