From 7f00d73004d963a90939c4232e47e740288371e3 Mon Sep 17 00:00:00 2001 From: Nithin Nayak Sujir Date: Wed, 22 Aug 2018 08:30:21 -0700 Subject: [PATCH 1/3] Spanner: Block nested transactions Cloud spanner does not support nested transactions. Use a thread-local flag to check and throw exception. --- .../com/google/cloud/spanner/SpannerImpl.java | 14 +++++++- .../spanner/TransactionRunnerImplTest.java | 29 +++++++++++++++- .../cloud/spanner/it/ITTransactionTest.java | 33 +++++++++++++++++++ 3 files changed, 74 insertions(+), 2 deletions(-) diff --git a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java index 850bf2898f57..f49b372c2bac 100644 --- a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java +++ b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java @@ -81,7 +81,6 @@ import io.opencensus.trace.Span; import io.opencensus.trace.Tracer; import io.opencensus.trace.Tracing; - import java.io.IOException; import java.io.Serializable; import java.util.AbstractList; @@ -1210,6 +1209,13 @@ public void execute(Runnable command) { @VisibleForTesting static class TransactionRunnerImpl implements SessionTransaction, TransactionRunner { + private static final ThreadLocal inTransaction = new ThreadLocal() { + @Override + protected Boolean initialValue() { + return false; + } + }; + /** Allow for testing of backoff logic */ static class Sleeper { void backoffSleep(Context context, long backoffMillis) { @@ -1238,12 +1244,18 @@ void backoffSleep(Context context, long backoffMillis) { @Nullable @Override public T run(TransactionCallable callable) { + if (inTransaction.get() == Boolean.TRUE) { + throw newSpannerException(ErrorCode.INTERNAL, "Nested transactions are not supported"); + } + try (Scope s = tracer.withSpan(span)) { + inTransaction.set(Boolean.TRUE); return runInternal(callable); } catch (RuntimeException e) { TraceUtil.endSpanWithFailure(span, e); throw e; } finally { + inTransaction.set(Boolean.FALSE); span.end(); } } diff --git a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/TransactionRunnerImplTest.java b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/TransactionRunnerImplTest.java index 9362814146ab..0872edf981c2 100644 --- a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/TransactionRunnerImplTest.java +++ b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/TransactionRunnerImplTest.java @@ -32,7 +32,6 @@ import io.grpc.Status; import io.grpc.StatusRuntimeException; import java.util.concurrent.atomic.AtomicInteger; - import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -144,6 +143,34 @@ public void runResourceExhaustedNoRetry() throws Exception { verify(txn).rollback(); } + private void runNestedTransaction() { + transactionRunner.run( + new TransactionCallable() { + @Override + public Void run(TransactionContext transaction) throws SpannerException { + transactionRunner.run( + new TransactionCallable() { + @Override + public Void run(TransactionContext transaction) throws Exception { + return null; + } + }); + return null; + } + }); + } + + @Test + public void nestedTransactionShouldThrowException() { + try { + runNestedTransaction(); + fail("Expected exception"); + } catch (SpannerException e) { + assertThat(e.getErrorCode()).isEqualTo(ErrorCode.INTERNAL); + assertThat(e.getMessage()).contains("not supported"); + } + } + private void runTransaction(final Exception exception) { transactionRunner.run( new TransactionCallable() { diff --git a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITTransactionTest.java b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITTransactionTest.java index 1bacea50d627..a01bdf03e140 100644 --- a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITTransactionTest.java +++ b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITTransactionTest.java @@ -349,4 +349,37 @@ public Void run(TransactionContext transaction) throws SpannerException { .getLong(0)) .isEqualTo(2); } + + private void doNestedTransaction() { + client + .readWriteTransaction() + .run( + new TransactionCallable() { + @Override + public Void run(TransactionContext transaction) throws SpannerException { + client + .readWriteTransaction() + .run( + new TransactionCallable() { + @Override + public Void run(TransactionContext transaction) throws Exception { + return null; + } + }); + + return null; + } + }); + } + + @Test + public void nestedTransactionShouldThrowException() { + try { + doNestedTransaction(); + fail("Expected exception"); + } catch (SpannerException e) { + assertThat(e.getErrorCode()).isEqualTo(ErrorCode.INTERNAL); + assertThat(e.getMessage()).contains("not supported"); + } + } } From 6c3919b1effb113ace106ee415bc9fd5ed8ebf86 Mon Sep 17 00:00:00 2001 From: Nithin Nayak Sujir Date: Mon, 27 Aug 2018 13:42:13 -0700 Subject: [PATCH 2/3] Rename inTransaction to hasPendingTransaction --- .../main/java/com/google/cloud/spanner/SpannerImpl.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java index f49b372c2bac..490cd2241385 100644 --- a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java +++ b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java @@ -1209,7 +1209,7 @@ public void execute(Runnable command) { @VisibleForTesting static class TransactionRunnerImpl implements SessionTransaction, TransactionRunner { - private static final ThreadLocal inTransaction = new ThreadLocal() { + private static final ThreadLocal hasPendingTransaction = new ThreadLocal() { @Override protected Boolean initialValue() { return false; @@ -1244,18 +1244,18 @@ void backoffSleep(Context context, long backoffMillis) { @Nullable @Override public T run(TransactionCallable callable) { - if (inTransaction.get() == Boolean.TRUE) { + if (hasPendingTransaction.get() == Boolean.TRUE) { throw newSpannerException(ErrorCode.INTERNAL, "Nested transactions are not supported"); } try (Scope s = tracer.withSpan(span)) { - inTransaction.set(Boolean.TRUE); + hasPendingTransaction.set(Boolean.TRUE); return runInternal(callable); } catch (RuntimeException e) { TraceUtil.endSpanWithFailure(span, e); throw e; } finally { - inTransaction.set(Boolean.FALSE); + hasPendingTransaction.set(Boolean.FALSE); span.end(); } } From a5568304cd31e928bc146b6275c1a7dc44188399 Mon Sep 17 00:00:00 2001 From: Nithin Nayak Sujir Date: Thu, 30 Aug 2018 14:51:21 -0700 Subject: [PATCH 3/3] Move hasPendingTransaction check into initTransaction() and setActive() This takes care of read, write and batch nested transactions. --- .../google-cloud-spanner/pom.xml | 2 + .../com/google/cloud/spanner/SpannerImpl.java | 28 ++++--- .../spanner/TransactionRunnerImplTest.java | 28 ------- .../cloud/spanner/it/ITTransactionTest.java | 83 ++++++++++++++++++- 4 files changed, 99 insertions(+), 42 deletions(-) diff --git a/google-cloud-clients/google-cloud-spanner/pom.xml b/google-cloud-clients/google-cloud-spanner/pom.xml index 3a563fe7eddd..ad5981934bf0 100644 --- a/google-cloud-clients/google-cloud-spanner/pom.xml +++ b/google-cloud-clients/google-cloud-spanner/pom.xml @@ -18,6 +18,7 @@ google-cloud-spanner + ${skipTests} @@ -27,6 +28,7 @@ 2.12.4 com.google.cloud.spanner.IntegrationTest + ${skipUnitTests} diff --git a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java index 490cd2241385..f17c97dbcf45 100644 --- a/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java +++ b/google-cloud-clients/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerImpl.java @@ -128,6 +128,19 @@ class SpannerImpl extends BaseService implements Spanner { private static final String QUERY = "CloudSpannerOperation.ExecuteStreamingQuery"; private static final String READ = "CloudSpannerOperation.ExecuteStreamingRead"; + private static final ThreadLocal hasPendingTransaction = new ThreadLocal() { + @Override + protected Boolean initialValue() { + return false; + } + }; + + private static void throwIfTransactionsPending() { + if (hasPendingTransaction.get() == Boolean.TRUE) { + throw newSpannerException(ErrorCode.INTERNAL, "Nested transactions are not supported"); + } + } + static { TraceUtil.exportSpans(CREATE_SESSION, DELETE_SESSION, BEGIN_TRANSACTION, COMMIT, QUERY, READ); } @@ -904,6 +917,8 @@ TransactionContextImpl newTransaction() { } T setActive(@Nullable T ctx) { + throwIfTransactionsPending(); + if (activeTransaction != null) { activeTransaction.invalidate(); } @@ -1209,13 +1224,6 @@ public void execute(Runnable command) { @VisibleForTesting static class TransactionRunnerImpl implements SessionTransaction, TransactionRunner { - private static final ThreadLocal hasPendingTransaction = new ThreadLocal() { - @Override - protected Boolean initialValue() { - return false; - } - }; - /** Allow for testing of backoff logic */ static class Sleeper { void backoffSleep(Context context, long backoffMillis) { @@ -1244,10 +1252,6 @@ void backoffSleep(Context context, long backoffMillis) { @Nullable @Override public T run(TransactionCallable callable) { - if (hasPendingTransaction.get() == Boolean.TRUE) { - throw newSpannerException(ErrorCode.INTERNAL, "Nested transactions are not supported"); - } - try (Scope s = tracer.withSpan(span)) { hasPendingTransaction.set(Boolean.TRUE); return runInternal(callable); @@ -1672,6 +1676,8 @@ ByteString getTransactionId() { } void initTransaction() { + throwIfTransactionsPending(); + // Since we only support synchronous calls, just block on "txnLock" while the RPC is in // flight. Note that we use the strategy of sending an explicit BeginTransaction() RPC, // rather than using the first read in the transaction to begin it implicitly. The chosen diff --git a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/TransactionRunnerImplTest.java b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/TransactionRunnerImplTest.java index 0872edf981c2..0cd804c6a89e 100644 --- a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/TransactionRunnerImplTest.java +++ b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/TransactionRunnerImplTest.java @@ -143,34 +143,6 @@ public void runResourceExhaustedNoRetry() throws Exception { verify(txn).rollback(); } - private void runNestedTransaction() { - transactionRunner.run( - new TransactionCallable() { - @Override - public Void run(TransactionContext transaction) throws SpannerException { - transactionRunner.run( - new TransactionCallable() { - @Override - public Void run(TransactionContext transaction) throws Exception { - return null; - } - }); - return null; - } - }); - } - - @Test - public void nestedTransactionShouldThrowException() { - try { - runNestedTransaction(); - fail("Expected exception"); - } catch (SpannerException e) { - assertThat(e.getErrorCode()).isEqualTo(ErrorCode.INTERNAL); - assertThat(e.getMessage()).contains("not supported"); - } - } - private void runTransaction(final Exception exception) { transactionRunner.run( new TransactionCallable() { diff --git a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITTransactionTest.java b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITTransactionTest.java index a01bdf03e140..139c75f66a04 100644 --- a/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITTransactionTest.java +++ b/google-cloud-clients/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITTransactionTest.java @@ -23,13 +23,17 @@ import com.google.cloud.Timestamp; import com.google.cloud.spanner.AbortedException; +import com.google.cloud.spanner.BatchClient; +import com.google.cloud.spanner.BatchReadOnlyTransaction; import com.google.cloud.spanner.Database; import com.google.cloud.spanner.DatabaseClient; import com.google.cloud.spanner.ErrorCode; import com.google.cloud.spanner.IntegrationTest; import com.google.cloud.spanner.IntegrationTestEnv; import com.google.cloud.spanner.Key; +import com.google.cloud.spanner.KeySet; import com.google.cloud.spanner.Mutation; +import com.google.cloud.spanner.PartitionOptions; import com.google.cloud.spanner.ReadContext; import com.google.cloud.spanner.ResultSet; import com.google.cloud.spanner.SpannerException; @@ -350,7 +354,7 @@ public Void run(TransactionContext transaction) throws SpannerException { .isEqualTo(2); } - private void doNestedTransaction() { + private void doNestedRwTransaction() { client .readWriteTransaction() .run( @@ -373,9 +377,82 @@ public Void run(TransactionContext transaction) throws Exception { } @Test - public void nestedTransactionShouldThrowException() { + public void nestedRwRwTransactionShouldThrowException() { try { - doNestedTransaction(); + doNestedRwTransaction(); + fail("Expected exception"); + } catch (SpannerException e) { + assertThat(e.getErrorCode()).isEqualTo(ErrorCode.INTERNAL); + assertThat(e.getMessage()).contains("not supported"); + } + } + + @Test + public void nestedRwRdTransactionShouldThrowException() { + try { + client + .readWriteTransaction() + .run( + new TransactionCallable() { + @Override + public Void run(TransactionContext transaction) throws SpannerException { + client + .readOnlyTransaction() + .getReadTimestamp(); + + return null; + } + }); + fail("Expected exception"); + } catch (SpannerException e) { + assertThat(e.getErrorCode()).isEqualTo(ErrorCode.INTERNAL); + assertThat(e.getMessage()).contains("not supported"); + } + } + + @Test + public void nestedRwBatchTransactionShouldThrowException() { + try { + client + .readWriteTransaction() + .run( + new TransactionCallable() { + @Override + public Void run(TransactionContext transaction) throws SpannerException { + BatchClient batchClient = env.getTestHelper().getBatchClient(db); + BatchReadOnlyTransaction batchTxn = batchClient + .batchReadOnlyTransaction(TimestampBound.strong()); + batchTxn.partitionReadUsingIndex( + PartitionOptions.getDefaultInstance(), + "Test", + "Index", + KeySet.all(), + Arrays.asList("Fingerprint")); + + return null; + } + }); + fail("Expected exception"); + } catch (SpannerException e) { + assertThat(e.getErrorCode()).isEqualTo(ErrorCode.INTERNAL); + assertThat(e.getMessage()).contains("not supported"); + } + } + + @Test + public void nestedRwSingleUseReadTransactionShouldThrowException() { + try { + client + .readWriteTransaction() + .run( + new TransactionCallable() { + @Override + public Void run(TransactionContext transaction) throws SpannerException { + client.singleUseReadOnlyTransaction(); + + return null; + } + }); fail("Expected exception"); } catch (SpannerException e) { assertThat(e.getErrorCode()).isEqualTo(ErrorCode.INTERNAL);