Skip to content

Commit

Permalink
Add detailed comment for allowNestedTransaction(), make test names cl…
Browse files Browse the repository at this point in the history
…earer
  • Loading branch information
nsujir committed Sep 14, 2018
1 parent 604e062 commit 72409fd
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -74,5 +74,22 @@ interface TransactionCallable<T> {
*/
Timestamp getCommitTimestamp();

/**
* Allows overriding the default behaviour of blocking nested transactions.
*
* Note that the client library does not maintain any information regarding the nesting structure.
* If an outer transaction fails and an inner transaction succeeds, upon retry of the outer
* transaction, the inner transaction will be re-executed.
*
* Use with care when certain that the inner transaction is idempotent. Avoid doing this when
* accessing the same db. There might be legitimate uses where access need to be made across DBs
* for instance.
*
* E.g. of nesting that is discouraged, see {@code nestedReadWriteTxnThrows}

This comment has been minimized.

Copy link
@snehashah16

snehashah16 Sep 14, 2018

Contributor

Not sure if we want to reference our tests in code comments.

This comment has been minimized.

Copy link
@nithinsujir

nithinsujir Sep 14, 2018

Thanks. Ya, it did occur to me as well, seemed like a better option than duplication.

* {@code nestedReadOnlyTxnThrows}, {@code nestedBatchTxnThrows},
* {@code nestedSingleUseReadTxnThrows}
*
* @return this object
*/
TransactionRunner allowNestedTransaction();
}
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ public Void run(TransactionContext transaction) throws Exception {
}

@Test
public void nestedRwRwTransactionShouldThrowException() {
public void nestedReadWriteTxnThrows() {
try {
doNestedRwTransaction();
fail("Expected exception");
Expand All @@ -388,7 +388,7 @@ public void nestedRwRwTransactionShouldThrowException() {
}

@Test
public void nestedRwRdTransactionShouldThrowException() {
public void nestedReadOnlyTxnThrows() {
try {
client
.readWriteTransaction()
Expand All @@ -411,7 +411,7 @@ public Void run(TransactionContext transaction) throws SpannerException {
}

@Test
public void nestedRwBatchTransactionShouldThrowException() {
public void nestedBatchTxnThrows() {
try {
client
.readWriteTransaction()
Expand Down Expand Up @@ -440,7 +440,7 @@ public Void run(TransactionContext transaction) throws SpannerException {
}

@Test
public void nestedRwSingleUseReadTransactionShouldThrowException() {
public void nestedSingleUseReadTxnThrows() {
try {
client
.readWriteTransaction()
Expand All @@ -461,7 +461,7 @@ public Void run(TransactionContext transaction) throws SpannerException {
}

@Test
public void nestedTxnShouldSucceedWhenAllowed() {
public void nestedTxnSucceedsWhenAllowed() {
client
.readWriteTransaction()
.allowNestedTransaction()
Expand Down

2 comments on commit 72409fd

@snehashah16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm !

@nithinsujir
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review. Looks a lot better than my first change.

@pongad PR is now ready to be merged.

Please sign in to comment.