Skip to content

Commit 94315ce

Browse files
rahul2393rahul2393
authored
fix(spanner): fail fast when inline-begin DML returns no transaction id (#13536)
Sync executeUpdate/executeBatchUpdate and async batchUpdateAsync left transactionIdFuture unresolved when a begin-requesting DML response carried no transaction metadata, so commit blocked on the 60s wait and surfaced DEADLINE_EXCEEDED. Mirror executeUpdateAsync: raise FAILED_PRECONDITION via onError so the future resolves and the transaction retries or propagates. Use getId().isEmpty() consistently and add regression coverage via a mock omit-transaction hook.
1 parent 0806ab6 commit 94315ce

3 files changed

Lines changed: 70 additions & 37 deletions

File tree

‎java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/TransactionRunnerImpl.java‎

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949
import com.google.spanner.v1.MultiplexedSessionPrecommitToken;
5050
import com.google.spanner.v1.RequestOptions;
5151
import com.google.spanner.v1.ResultSet;
52+
import com.google.spanner.v1.ResultSetMetadata;
5253
import com.google.spanner.v1.ResultSetStats;
5354
import com.google.spanner.v1.RollbackRequest;
5455
import com.google.spanner.v1.Transaction;
@@ -742,7 +743,7 @@ options, getPreviousTransactionId())))
742743
@Override
743744
public void onTransactionMetadata(Transaction transaction, boolean shouldIncludeId) {
744745
Preconditions.checkNotNull(transaction);
745-
if (transaction.getId() != ByteString.EMPTY) {
746+
if (!transaction.getId().isEmpty()) {
746747
// A transaction has been returned by a statement that was executed. Set the id of the
747748
// transaction on this instance and release the lock to allow other statements to proceed.
748749
if ((transactionIdFuture == null || !this.transactionIdFuture.isDone())
@@ -757,6 +758,18 @@ public void onTransactionMetadata(Transaction transaction, boolean shouldInclude
757758
}
758759
}
759760

761+
private boolean hasNonEmptyTransactionId(ResultSetMetadata metadata) {
762+
return metadata.hasTransaction() && !metadata.getTransaction().getId().isEmpty();
763+
}
764+
765+
private void throwIfBeginDidNotReturnTransaction(
766+
TransactionSelector transactionSelector, boolean sawNonEmptyTransactionId) {
767+
if (transactionSelector.hasBegin() && !sawNonEmptyTransactionId) {
768+
throw SpannerExceptionFactory.newSpannerException(
769+
ErrorCode.FAILED_PRECONDITION, NO_TRANSACTION_RETURNED_MSG);
770+
}
771+
}
772+
760773
/**
761774
* In read-write transactions, the precommit token with the highest sequence number from this
762775
* transaction attempt will be tracked and included in the
@@ -980,14 +993,16 @@ private ResultSet internalExecuteUpdate(
980993
com.google.spanner.v1.ResultSet resultSet =
981994
rpc.executeQuery(builder.build(), getTransactionChannelHint(), isRouteToLeader());
982995
session.markUsed(clock.instant());
983-
if (resultSet.getMetadata().hasTransaction()) {
996+
boolean sawNonEmptyTransactionId = hasNonEmptyTransactionId(resultSet.getMetadata());
997+
if (sawNonEmptyTransactionId) {
984998
onTransactionMetadata(
985999
resultSet.getMetadata().getTransaction(), builder.getTransaction().hasBegin());
9861000
}
9871001
if (!resultSet.hasStats()) {
9881002
throw new IllegalArgumentException(
9891003
"DML response missing stats possibly due to non-DML statement as input");
9901004
}
1005+
throwIfBeginDidNotReturnTransaction(builder.getTransaction(), sawNonEmptyTransactionId);
9911006
if (resultSet.hasPrecommitToken()) {
9921007
onPrecommitToken(resultSet.getPrecommitToken());
9931008
}
@@ -1037,12 +1052,8 @@ public ApiFuture<Long> executeUpdateAsync(Statement statement, UpdateOption... u
10371052
ErrorCode.INVALID_ARGUMENT,
10381053
"DML response missing stats possibly due to non-DML statement as input");
10391054
}
1040-
if (builder.getTransaction().hasBegin()
1041-
&& !(input.getMetadata().hasTransaction()
1042-
&& input.getMetadata().getTransaction().getId() != ByteString.EMPTY)) {
1043-
throw SpannerExceptionFactory.newSpannerException(
1044-
ErrorCode.FAILED_PRECONDITION, NO_TRANSACTION_RETURNED_MSG);
1045-
}
1055+
throwIfBeginDidNotReturnTransaction(
1056+
builder.getTransaction(), hasNonEmptyTransactionId(input.getMetadata()));
10461057
// For standard DML, using the exact row count.
10471058
return input.getStats().getRowCountExact();
10481059
},
@@ -1116,9 +1127,11 @@ public long[] batchUpdate(Iterable<Statement> statements, UpdateOption... update
11161127
rpc.executeBatchDml(builder.build(), getTransactionChannelHint());
11171128
session.markUsed(clock.instant());
11181129
long[] results = new long[response.getResultSetsCount()];
1130+
boolean sawNonEmptyTransactionId = false;
11191131
for (int i = 0; i < response.getResultSetsCount(); ++i) {
11201132
results[i] = response.getResultSets(i).getStats().getRowCountExact();
1121-
if (response.getResultSets(i).getMetadata().hasTransaction()) {
1133+
if (hasNonEmptyTransactionId(response.getResultSets(i).getMetadata())) {
1134+
sawNonEmptyTransactionId = true;
11221135
onTransactionMetadata(
11231136
response.getResultSets(i).getMetadata().getTransaction(),
11241137
builder.getTransaction().hasBegin());
@@ -1139,6 +1152,7 @@ public long[] batchUpdate(Iterable<Statement> statements, UpdateOption... update
11391152
response.getStatus().getMessage(),
11401153
results);
11411154
}
1155+
throwIfBeginDidNotReturnTransaction(builder.getTransaction(), sawNonEmptyTransactionId);
11421156
return results;
11431157
} catch (Throwable e) {
11441158
throw onError(
@@ -1187,9 +1201,11 @@ public ApiFuture<long[]> batchUpdateAsync(
11871201
response,
11881202
batchDmlResponse -> {
11891203
long[] results = new long[batchDmlResponse.getResultSetsCount()];
1204+
boolean sawNonEmptyTransactionId = false;
11901205
for (int i = 0; i < batchDmlResponse.getResultSetsCount(); ++i) {
11911206
results[i] = batchDmlResponse.getResultSets(i).getStats().getRowCountExact();
1192-
if (batchDmlResponse.getResultSets(i).getMetadata().hasTransaction()) {
1207+
if (hasNonEmptyTransactionId(batchDmlResponse.getResultSets(i).getMetadata())) {
1208+
sawNonEmptyTransactionId = true;
11931209
onTransactionMetadata(
11941210
batchDmlResponse.getResultSets(i).getMetadata().getTransaction(),
11951211
builder.getTransaction().hasBegin());
@@ -1208,6 +1224,8 @@ public ApiFuture<long[]> batchUpdateAsync(
12081224
batchDmlResponse.getStatus().getMessage(),
12091225
results);
12101226
}
1227+
throwIfBeginDidNotReturnTransaction(
1228+
builder.getTransaction(), sawNonEmptyTransactionId);
12111229
return results;
12121230
},
12131231
MoreExecutors.directExecutor());

‎java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/InlineBeginTransactionTest.java‎

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1677,6 +1677,10 @@ static void runWithIgnoreInlineBegin(Runnable runnable) {
16771677
}
16781678
}
16791679

1680+
static void useShortTransactionWait(TransactionContext transaction) {
1681+
((TransactionContextImpl) transaction).waitForTransactionTimeoutMillis = 1L;
1682+
}
1683+
16801684
@Test
16811685
public void testQueryWithInlineBeginDidNotReturnTransaction() {
16821686
runWithIgnoreInlineBegin(
@@ -1738,7 +1742,11 @@ public void testUpdateWithInlineBeginDidNotReturnTransaction() {
17381742
() ->
17391743
client
17401744
.readWriteTransaction()
1741-
.run(transaction -> transaction.executeUpdate(UPDATE_STATEMENT)));
1745+
.run(
1746+
transaction -> {
1747+
useShortTransactionWait(transaction);
1748+
return transaction.executeUpdate(UPDATE_STATEMENT);
1749+
}));
17421750
assertEquals(ErrorCode.FAILED_PRECONDITION, e.getErrorCode());
17431751
assertThat(e.getMessage()).contains(AbstractReadContext.NO_TRANSACTION_RETURNED_MSG);
17441752
assertThat(countRequests(BeginTransactionRequest.class)).isEqualTo(0);
@@ -1760,6 +1768,7 @@ public void testBatchUpdateWithInlineBeginDidNotReturnTransaction() {
17601768
.readWriteTransaction()
17611769
.run(
17621770
transaction -> {
1771+
useShortTransactionWait(transaction);
17631772
transaction.batchUpdate(
17641773
Collections.singletonList(UPDATE_STATEMENT));
17651774
return null;
@@ -1831,9 +1840,11 @@ public void testUpdateAsyncWithInlineBeginDidNotReturnTransaction() {
18311840
client
18321841
.readWriteTransaction()
18331842
.run(
1834-
transaction ->
1835-
SpannerApiFutures.get(
1836-
transaction.executeUpdateAsync(UPDATE_STATEMENT))));
1843+
transaction -> {
1844+
useShortTransactionWait(transaction);
1845+
return SpannerApiFutures.get(
1846+
transaction.executeUpdateAsync(UPDATE_STATEMENT));
1847+
}));
18371848
assertEquals(ErrorCode.FAILED_PRECONDITION, e.getErrorCode());
18381849
assertThat(e.getMessage()).contains(AbstractReadContext.NO_TRANSACTION_RETURNED_MSG);
18391850
assertThat(countRequests(BeginTransactionRequest.class)).isEqualTo(0);
@@ -1854,10 +1865,12 @@ public void testBatchUpdateAsyncWithInlineBeginDidNotReturnTransaction() {
18541865
client
18551866
.readWriteTransaction()
18561867
.run(
1857-
transaction ->
1858-
SpannerApiFutures.get(
1859-
transaction.batchUpdateAsync(
1860-
Collections.singletonList(UPDATE_STATEMENT)))));
1868+
transaction -> {
1869+
useShortTransactionWait(transaction);
1870+
return SpannerApiFutures.get(
1871+
transaction.batchUpdateAsync(
1872+
Collections.singletonList(UPDATE_STATEMENT)));
1873+
}));
18611874
assertEquals(ErrorCode.FAILED_PRECONDITION, e.getErrorCode());
18621875
assertThat(e.getMessage()).contains(AbstractReadContext.NO_TRANSACTION_RETURNED_MSG);
18631876
assertThat(countRequests(BeginTransactionRequest.class)).isEqualTo(0);

‎java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/MockSpannerServiceImpl.java‎

Lines changed: 21 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -813,6 +813,10 @@ public void setIgnoreInlineBeginRequest(boolean ignore) {
813813
ignoreInlineBeginRequest.set(ignore);
814814
}
815815

816+
private boolean shouldOmitInlineBeginTransaction(TransactionSelector transactionSelector) {
817+
return ignoreInlineBeginRequest.get() && transactionSelector.hasBegin();
818+
}
819+
816820
public void freeze() {
817821
synchronized (lock) {
818822
freezeLock = new CountDownLatch(1);
@@ -1100,12 +1104,12 @@ public void executeSql(ExecuteSqlRequest request, StreamObserver<ResultSet> resp
11001104
.setRowCountExact(result.getUpdateCount())
11011105
.build())
11021106
.setMetadata(
1103-
ResultSetMetadata.newBuilder()
1104-
.setTransaction(
1105-
ignoreInlineBeginRequest.get()
1106-
? Transaction.getDefaultInstance()
1107-
: Transaction.newBuilder().setId(transactionId).build())
1108-
.build());
1107+
shouldOmitInlineBeginTransaction(request.getTransaction())
1108+
? ResultSetMetadata.getDefaultInstance()
1109+
: ResultSetMetadata.newBuilder()
1110+
.setTransaction(
1111+
Transaction.newBuilder().setId(transactionId).build())
1112+
.build());
11091113
if (session.getMultiplexed() && isReadWriteTransaction(transactionId)) {
11101114
resultSetBuilder.setPrecommitToken(getResultSetPrecommitToken(transactionId));
11111115
}
@@ -1131,13 +1135,12 @@ private void returnResultSet(
11311135
Session session) {
11321136
ResultSetMetadata metadata = resultSet.getMetadata();
11331137
if (transactionId != null) {
1134-
metadata =
1135-
metadata.toBuilder()
1136-
.setTransaction(
1137-
ignoreInlineBeginRequest.get()
1138-
? Transaction.getDefaultInstance()
1139-
: Transaction.newBuilder().setId(transactionId).build())
1140-
.build();
1138+
if (!shouldOmitInlineBeginTransaction(transactionSelector)) {
1139+
metadata =
1140+
metadata.toBuilder()
1141+
.setTransaction(Transaction.newBuilder().setId(transactionId).build())
1142+
.build();
1143+
}
11411144
} else if (transactionSelector.hasBegin() || transactionSelector.hasSingleUse()) {
11421145
Transaction transaction = getTemporaryTransactionOrNull(transactionSelector);
11431146
metadata = metadata.toBuilder().setTransaction(transaction).build();
@@ -1234,12 +1237,11 @@ public void executeBatchDml(
12341237
ResultSet.newBuilder()
12351238
.setStats(ResultSetStats.newBuilder().setRowCountExact(updateCount).build())
12361239
.setMetadata(
1237-
ResultSetMetadata.newBuilder()
1238-
.setTransaction(
1239-
ignoreInlineBeginRequest.get()
1240-
? Transaction.getDefaultInstance()
1241-
: Transaction.newBuilder().setId(transactionId).build())
1242-
.build())
1240+
shouldOmitInlineBeginTransaction(request.getTransaction())
1241+
? ResultSetMetadata.getDefaultInstance()
1242+
: ResultSetMetadata.newBuilder()
1243+
.setTransaction(Transaction.newBuilder().setId(transactionId).build())
1244+
.build())
12431245
.build());
12441246
}
12451247
builder.setStatus(status);

0 commit comments

Comments
 (0)