Skip to content

Commit a2da391

Browse files
brfrn169brfrn169
committed
Allow multiple calls to rollback()
1 parent e7b9795 commit a2da391

File tree

5 files changed

+146
-14
lines changed

5 files changed

+146
-14
lines changed

‎core/src/main/java/com/scalar/db/common/StateManagedDistributedTransactionManager.java‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -144,9 +144,12 @@ public void commit() throws CommitException, UnknownTransactionStatusException {
144144

145145
@Override
146146
public void rollback() throws RollbackException {
147-
if (status == Status.COMMITTED || status == Status.ROLLED_BACK) {
147+
if (status == Status.ROLLED_BACK) {
148+
return;
149+
}
150+
if (status == Status.COMMITTED) {
148151
throw new IllegalStateException(
149-
CoreError.TRANSACTION_ALREADY_COMMITTED_OR_ROLLED_BACK.buildMessage(status));
152+
CoreError.TRANSACTION_ALREADY_COMMITTED.buildMessage(status));
150153
}
151154
try {
152155
super.rollback();
@@ -157,9 +160,12 @@ public void rollback() throws RollbackException {
157160

158161
@Override
159162
public void abort() throws AbortException {
160-
if (status == Status.COMMITTED || status == Status.ROLLED_BACK) {
163+
if (status == Status.ROLLED_BACK) {
164+
return;
165+
}
166+
if (status == Status.COMMITTED) {
161167
throw new IllegalStateException(
162-
CoreError.TRANSACTION_ALREADY_COMMITTED_OR_ROLLED_BACK.buildMessage(status));
168+
CoreError.TRANSACTION_ALREADY_COMMITTED.buildMessage(status));
163169
}
164170
try {
165171
super.abort();

‎core/src/main/java/com/scalar/db/common/StateManagedTwoPhaseCommitTransactionManager.java‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -181,9 +181,12 @@ public void commit() throws CommitException, UnknownTransactionStatusException {
181181

182182
@Override
183183
public void rollback() throws RollbackException {
184-
if (status == Status.COMMITTED || status == Status.ROLLED_BACK) {
184+
if (status == Status.ROLLED_BACK) {
185+
return;
186+
}
187+
if (status == Status.COMMITTED) {
185188
throw new IllegalStateException(
186-
CoreError.TRANSACTION_ALREADY_COMMITTED_OR_ROLLED_BACK.buildMessage(status));
189+
CoreError.TRANSACTION_ALREADY_COMMITTED.buildMessage(status));
187190
}
188191
try {
189192
super.rollback();
@@ -194,9 +197,12 @@ public void rollback() throws RollbackException {
194197

195198
@Override
196199
public void abort() throws AbortException {
197-
if (status == Status.COMMITTED || status == Status.ROLLED_BACK) {
200+
if (status == Status.ROLLED_BACK) {
201+
return;
202+
}
203+
if (status == Status.COMMITTED) {
198204
throw new IllegalStateException(
199-
CoreError.TRANSACTION_ALREADY_COMMITTED_OR_ROLLED_BACK.buildMessage(status));
205+
CoreError.TRANSACTION_ALREADY_COMMITTED.buildMessage(status));
200206
}
201207
try {
202208
super.abort();

‎core/src/main/java/com/scalar/db/common/error/CoreError.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -214,10 +214,10 @@ public enum CoreError implements ScalarDbError {
214214
Category.USER_ERROR, "0042", "Invalid ID specified. ID: %d", "", ""),
215215
TRANSACTION_NOT_ACTIVE(
216216
Category.USER_ERROR, "0043", "The transaction is not active. Status: %s", "", ""),
217-
TRANSACTION_ALREADY_COMMITTED_OR_ROLLED_BACK(
217+
TRANSACTION_ALREADY_COMMITTED(
218218
Category.USER_ERROR,
219219
"0044",
220-
"The transaction has already been committed or rolled back. Status: %s",
220+
"The transaction has already been committed. Status: %s",
221221
"",
222222
""),
223223
TRANSACTION_NOT_PREPARED(

‎core/src/test/java/com/scalar/db/common/StateManagedDistributedTransactionManagerTest.java‎

Lines changed: 49 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77
import static org.mockito.ArgumentMatchers.anyString;
88
import static org.mockito.Mockito.doThrow;
99
import static org.mockito.Mockito.mock;
10+
import static org.mockito.Mockito.only;
11+
import static org.mockito.Mockito.verify;
1012
import static org.mockito.Mockito.when;
1113

1214
import com.scalar.db.api.Delete;
@@ -21,6 +23,7 @@
2123
import com.scalar.db.api.SerializableStrategy;
2224
import com.scalar.db.api.Update;
2325
import com.scalar.db.api.Upsert;
26+
import com.scalar.db.exception.transaction.AbortException;
2427
import com.scalar.db.exception.transaction.CommitException;
2528
import com.scalar.db.exception.transaction.RollbackException;
2629
import com.scalar.db.exception.transaction.TransactionException;
@@ -308,12 +311,56 @@ public void rollback_AfterCommit_ShouldThrowIllegalStateException()
308311
}
309312

310313
@Test
311-
public void rollback_Twice_ShouldThrowIllegalStateException() throws RollbackException {
314+
public void rollback_Twice_ShouldNotThrowAnyExceptionAndCallWrappedTransactionRollbackOnlyOnce()
315+
throws RollbackException {
312316
// Arrange
313317
transaction.rollback();
314318

315319
// Act Assert
316-
assertThatThrownBy(() -> transaction.rollback()).isInstanceOf(IllegalStateException.class);
320+
assertThatCode(() -> transaction.rollback()).doesNotThrowAnyException();
321+
322+
verify(wrappedTransaction, only()).rollback();
323+
}
324+
325+
@Test
326+
public void abort_ShouldNotThrowAnyException() {
327+
// Arrange
328+
329+
// Act Assert
330+
assertThatCode(() -> transaction.abort()).doesNotThrowAnyException();
331+
}
332+
333+
@Test
334+
public void abort_AfterCommitFailed_ShouldNotThrowAnyException()
335+
throws CommitException, UnknownTransactionStatusException {
336+
// Arrange
337+
doThrow(CommitException.class).when(wrappedTransaction).commit();
338+
assertThatThrownBy(() -> transaction.commit()).isInstanceOf(CommitException.class);
339+
340+
// Act Assert
341+
assertThatCode(() -> transaction.abort()).doesNotThrowAnyException();
342+
}
343+
344+
@Test
345+
public void abort_AfterCommit_ShouldThrowIllegalStateException()
346+
throws CommitException, UnknownTransactionStatusException {
347+
// Arrange
348+
transaction.commit();
349+
350+
// Act Assert
351+
assertThatThrownBy(() -> transaction.abort()).isInstanceOf(IllegalStateException.class);
352+
}
353+
354+
@Test
355+
public void abort_Twice_ShouldNotThrowAnyExceptionAndCallWrappedTransactionAbortOnlyOnce()
356+
throws AbortException {
357+
// Arrange
358+
transaction.abort();
359+
360+
// Act Assert
361+
assertThatCode(() -> transaction.abort()).doesNotThrowAnyException();
362+
363+
verify(wrappedTransaction, only()).abort();
317364
}
318365
}
319366
}

‎core/src/test/java/com/scalar/db/common/StateManagedTwoPhaseCommitTransactionManagerTest.java‎

Lines changed: 75 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
import static org.mockito.ArgumentMatchers.anyString;
77
import static org.mockito.Mockito.doThrow;
88
import static org.mockito.Mockito.mock;
9+
import static org.mockito.Mockito.only;
10+
import static org.mockito.Mockito.verify;
911
import static org.mockito.Mockito.when;
1012

1113
import com.scalar.db.api.Delete;
@@ -18,6 +20,7 @@
1820
import com.scalar.db.api.TwoPhaseCommitTransactionManager;
1921
import com.scalar.db.api.Update;
2022
import com.scalar.db.api.Upsert;
23+
import com.scalar.db.exception.transaction.AbortException;
2124
import com.scalar.db.exception.transaction.CommitException;
2225
import com.scalar.db.exception.transaction.PreparationException;
2326
import com.scalar.db.exception.transaction.RollbackException;
@@ -385,12 +388,82 @@ public void rollback_AfterCommit_ShouldThrowIllegalStateException()
385388
}
386389

387390
@Test
388-
public void rollback_Twice_ShouldThrowIllegalStateException() throws RollbackException {
391+
public void rollback_Twice_ShouldNotThrowAnyExceptionAndCallWrappedTransactionRollbackOnlyOnce()
392+
throws RollbackException {
389393
// Arrange
390394
transaction.rollback();
391395

392396
// Act Assert
393-
assertThatThrownBy(() -> transaction.rollback()).isInstanceOf(IllegalStateException.class);
397+
assertThatCode(() -> transaction.rollback()).doesNotThrowAnyException();
398+
399+
verify(wrappedTransaction, only()).rollback();
400+
}
401+
402+
@Test
403+
public void abort_ShouldNotThrowAnyException() {
404+
// Arrange
405+
406+
// Act Assert
407+
assertThatCode(() -> transaction.abort()).doesNotThrowAnyException();
408+
}
409+
410+
@Test
411+
public void abort_AfterCommitFailed_ShouldNotThrowAnyException()
412+
throws PreparationException, CommitException, UnknownTransactionStatusException {
413+
// Arrange
414+
doThrow(CommitException.class).when(wrappedTransaction).commit();
415+
416+
transaction.prepare();
417+
assertThatThrownBy(() -> transaction.commit()).isInstanceOf(CommitException.class);
418+
419+
// Act Assert
420+
assertThatCode(() -> transaction.abort()).doesNotThrowAnyException();
421+
}
422+
423+
@Test
424+
public void abort_AfterPrepareFailed_ShouldNotThrowAnyException() throws PreparationException {
425+
// Arrange
426+
doThrow(PreparationException.class).when(wrappedTransaction).prepare();
427+
assertThatThrownBy(() -> transaction.prepare()).isInstanceOf(PreparationException.class);
428+
429+
// Act Assert
430+
assertThatCode(() -> transaction.abort()).doesNotThrowAnyException();
431+
}
432+
433+
@Test
434+
public void abort_AfterValidateFailed_ShouldNotThrowAnyException()
435+
throws PreparationException, ValidationException {
436+
// Arrange
437+
doThrow(ValidationException.class).when(wrappedTransaction).validate();
438+
439+
transaction.prepare();
440+
assertThatThrownBy(() -> transaction.validate()).isInstanceOf(ValidationException.class);
441+
442+
// Act Assert
443+
assertThatCode(() -> transaction.abort()).doesNotThrowAnyException();
444+
}
445+
446+
@Test
447+
public void abort_AfterCommit_ShouldThrowIllegalStateException()
448+
throws PreparationException, CommitException, UnknownTransactionStatusException {
449+
// Arrange
450+
transaction.prepare();
451+
transaction.commit();
452+
453+
// Act Assert
454+
assertThatThrownBy(() -> transaction.abort()).isInstanceOf(IllegalStateException.class);
455+
}
456+
457+
@Test
458+
public void abort_Twice_ShouldNotThrowAnyExceptionAndCallWrappedTransactionAbortOnlyOnce()
459+
throws AbortException {
460+
// Arrange
461+
transaction.abort();
462+
463+
// Act Assert
464+
assertThatCode(() -> transaction.abort()).doesNotThrowAnyException();
465+
466+
verify(wrappedTransaction, only()).abort();
394467
}
395468
}
396469
}

0 commit comments

Comments
 (0)