Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: add CommitStats to Connection API #608

Merged
merged 49 commits into from Feb 24, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
3872199
feat!: add support for CommitStats
olavloite Oct 24, 2020
def8768
fix: add clirr ignored differences
olavloite Oct 24, 2020
5372dae
Merge branch 'master' into commit-stats2
olavloite Oct 31, 2020
34fbda6
fix: error message should start with getCommitResponse
olavloite Oct 31, 2020
8f61c2a
Merge branch 'master' into commit-stats2
olavloite Nov 6, 2020
47b57ea
feat: add CommitStats to Connection API
olavloite Nov 6, 2020
a9014bc
fix: combine commit timestamp and response methods
olavloite Nov 6, 2020
1063ced
fix: clirr-diff and add some tests
olavloite Nov 7, 2020
2f56c29
fix: remove profile change from this PR
olavloite Nov 9, 2020
041b34d
Merge branch 'master' into commit-stats2
olavloite Dec 5, 2020
df7273a
Merge branch 'master' into connection-api-commit-stats
olavloite Dec 5, 2020
8299054
fix: remove overload delay
olavloite Dec 10, 2020
9b710a7
Merge branch 'master' into commit-stats2
olavloite Dec 10, 2020
9a2e555
Merge branch 'master' into connection-api-commit-stats
olavloite Dec 11, 2020
57a10a8
fix: remove overload delay
olavloite Dec 11, 2020
88c2d83
Merge branch 'commit-stats2' into connection-api-commit-stats
olavloite Dec 11, 2020
043555d
fix: fix test failure
olavloite Dec 11, 2020
8c15641
Merge branch 'master' into commit-stats2
olavloite Jan 23, 2021
919cf02
chore: cleanup after merge
olavloite Jan 23, 2021
97ec917
fix: update copyright years of new files
olavloite Jan 23, 2021
afeb0fd
test: fix flaky test
olavloite Jan 23, 2021
8524526
test: skip commit stats tests on emulator
olavloite Jan 23, 2021
06b3e22
test: missed one commit stats tests against emulator
olavloite Jan 23, 2021
1c17d16
test: skip another emulator test
olavloite Jan 23, 2021
664b87d
test: add missing test cases
olavloite Jan 23, 2021
6573a0f
fix: address review comments
olavloite Jan 30, 2021
0b448c3
Merge branch 'master' into commit-stats2
olavloite Feb 1, 2021
83039fb
Merge branch 'master' into commit-stats2
olavloite Feb 3, 2021
57ea714
chore: use junit assertion instead of truth
olavloite Feb 3, 2021
74f5951
Merge branch 'master' into connection-api-commit-stats
olavloite Feb 3, 2021
1b077e4
chore: cleanup after merge
olavloite Feb 3, 2021
ea061f8
Merge branch 'commit-stats2' into connection-api-commit-stats
olavloite Feb 3, 2021
51fb048
fix: address review comments
olavloite Feb 3, 2021
66c5f88
chore: replace truth asserts with junit asserts
olavloite Feb 4, 2021
bd9bf11
Merge branch 'commit-stats2' into connection-api-commit-stats
olavloite Feb 4, 2021
b351d08
test: skip CommitStats integration tests on emulator
olavloite Feb 4, 2021
da0e434
fix: address review comments
olavloite Feb 4, 2021
1d17973
chore: replace truth assertions with junit
olavloite Feb 5, 2021
a2531cb
Merge branch 'commit-stats2' into connection-api-commit-stats
olavloite Feb 5, 2021
c01152d
test: add additional tests + replace truth with junit assertions
olavloite Feb 5, 2021
bf0a7bd
fix: fix wrong error code
olavloite Feb 5, 2021
48ef21b
chore: cleanup test and variable names
olavloite Feb 8, 2021
1f33c42
fix: rename test method and variables
olavloite Feb 9, 2021
7d45ace
fix: address review comments
olavloite Feb 16, 2021
fcb9eda
Merge branch 'commit-stats2' into connection-api-commit-stats
olavloite Feb 16, 2021
682b849
Merge branch 'master' into connection-api-commit-stats
olavloite Feb 23, 2021
0e341a9
chore: remove unnecessary changes + JUnit assertions
olavloite Feb 23, 2021
7540c31
fix: remove reference to overload_delay
olavloite Feb 23, 2021
f86255e
chore: address review comments
olavloite Feb 24, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 17 additions & 0 deletions google-cloud-spanner/clirr-ignored-differences.xml
Expand Up @@ -476,6 +476,23 @@
<method>com.google.cloud.spanner.CommitResponse getCommitResponse()</method>
</difference>

<!-- Support for Commit Stats in Connection API -->
<difference>
<differenceType>7012</differenceType>
<className>com/google/cloud/spanner/connection/Connection</className>
<method>com.google.cloud.spanner.CommitResponse getCommitResponse()</method>
</difference>
<difference>
<differenceType>7012</differenceType>
<className>com/google/cloud/spanner/connection/Connection</className>
<method>boolean isReturnCommitStats()</method>
</difference>
<difference>
<differenceType>7012</differenceType>
<className>com/google/cloud/spanner/connection/Connection</className>
<method>void setReturnCommitStats(boolean)</method>
</difference>

<!-- PITR -->
<difference>
<differenceType>7013</differenceType>
Expand Down
Expand Up @@ -41,6 +41,11 @@ public Timestamp getCommitTimestamp() {
return Timestamp.fromProto(proto.getCommitTimestamp());
}

/** @return true if the {@link CommitResponse} includes {@link CommitStats} */
public boolean hasCommitStats() {
olavloite marked this conversation as resolved.
Show resolved Hide resolved
return proto.hasCommitStats();
}

/**
* Commit statistics are returned by a read/write transaction if specifically requested by passing
* in {@link Options#commitStats()} to the transaction.
Expand Down
Expand Up @@ -22,6 +22,7 @@
import com.google.cloud.spanner.AbortedDueToConcurrentModificationException;
import com.google.cloud.spanner.AbortedException;
import com.google.cloud.spanner.AsyncResultSet;
import com.google.cloud.spanner.CommitResponse;
import com.google.cloud.spanner.ErrorCode;
import com.google.cloud.spanner.Mutation;
import com.google.cloud.spanner.Options.QueryOption;
Expand Down Expand Up @@ -438,6 +439,15 @@ public interface Connection extends AutoCloseable {
*/
String getOptimizerVersion();

/**
* Sets whether this connection should request commit statistics from Cloud Spanner for read/write
* transactions and DML statements in autocommit mode.
*/
void setReturnCommitStats(boolean returnCommitStats);

/** @return true if this connection requests commit statistics from Cloud Spanner */
boolean isReturnCommitStats();

/**
* Commits the current transaction of this connection. All mutations that have been buffered
* during the current transaction will be written to the database.
Expand Down Expand Up @@ -623,15 +633,26 @@ public interface Connection extends AutoCloseable {

/**
* @return the commit timestamp of the last {@link TransactionMode#READ_WRITE_TRANSACTION}
* transaction. This method will throw a {@link SpannerException} if there is no last {@link
* TransactionMode#READ_WRITE_TRANSACTION} transaction (i.e. the last transaction was a {@link
* TransactionMode#READ_ONLY_TRANSACTION}), or if the last {@link
* TransactionMode#READ_WRITE_TRANSACTION} transaction rolled back. It will also throw a
* {@link SpannerException} if the last {@link TransactionMode#READ_WRITE_TRANSACTION}
* transaction was empty when committed.
* transaction. This method throws a {@link SpannerException} if there is no last {@link
* TransactionMode#READ_WRITE_TRANSACTION} transaction. That is, if the last transaction was a
* {@link TransactionMode#READ_ONLY_TRANSACTION}), or if the last {@link
* TransactionMode#READ_WRITE_TRANSACTION} transaction rolled back. It also throws a {@link
* SpannerException} if the last {@link TransactionMode#READ_WRITE_TRANSACTION} transaction
* was empty when committed.
*/
Timestamp getCommitTimestamp();

/**
* @return the {@link CommitResponse} of the last {@link TransactionMode#READ_WRITE_TRANSACTION}
* transaction. This method throws a {@link SpannerException} if there is no last {@link
* TransactionMode#READ_WRITE_TRANSACTION} transaction. That is, if the last transaction was a
* {@link TransactionMode#READ_ONLY_TRANSACTION}), or if the last {@link
* TransactionMode#READ_WRITE_TRANSACTION} transaction rolled back. It also throws a {@link
* SpannerException} if the last {@link TransactionMode#READ_WRITE_TRANSACTION} transaction
* was empty when committed.
*/
CommitResponse getCommitResponse();

/**
* Starts a new DDL batch on this connection. A DDL batch allows several DDL statements to be
* grouped into a batch that can be executed as a group. DDL statements that are issued during the
Expand Down
Expand Up @@ -22,6 +22,7 @@
import com.google.api.core.ApiFutures;
import com.google.cloud.Timestamp;
import com.google.cloud.spanner.AsyncResultSet;
import com.google.cloud.spanner.CommitResponse;
import com.google.cloud.spanner.DatabaseClient;
import com.google.cloud.spanner.ErrorCode;
import com.google.cloud.spanner.Mutation;
Expand Down Expand Up @@ -179,6 +180,7 @@ static UnitOfWorkType of(TransactionMode transactionMode) {
private DatabaseClient dbClient;
private boolean autocommit;
private boolean readOnly;
private boolean returnCommitStats;

private UnitOfWork currentUnitOfWork = null;
/**
Expand Down Expand Up @@ -213,6 +215,7 @@ static UnitOfWorkType of(TransactionMode transactionMode) {
this.readOnly = options.isReadOnly();
this.autocommit = options.isAutocommit();
this.queryOptions = this.queryOptions.toBuilder().mergeFrom(options.getQueryOptions()).build();
this.returnCommitStats = options.isReturnCommitStats();
this.ddlClient = createDdlClient();
setDefaultTransactionOptions();
}
Expand All @@ -237,6 +240,7 @@ static UnitOfWorkType of(TransactionMode transactionMode) {
this.dbClient = dbClient;
setReadOnly(options.isReadOnly());
setAutocommit(options.isAutocommit());
setReturnCommitStats(options.isReturnCommitStats());
setDefaultTransactionOptions();
}

Expand Down Expand Up @@ -580,6 +584,31 @@ Timestamp getCommitTimestampOrNull() {
: this.currentUnitOfWork.getCommitTimestampOrNull();
}

@Override
public CommitResponse getCommitResponse() {
ConnectionPreconditions.checkState(!isClosed(), CLOSED_ERROR_MSG);
ConnectionPreconditions.checkState(
this.currentUnitOfWork != null, "There is no transaction on this connection");
return this.currentUnitOfWork.getCommitResponse();
}

CommitResponse getCommitResponseOrNull() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't see why both getCommitResponseOrNull and getCommitResponse exist. Why not just getCommitResponse that returns null if there's no unit of work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These two methods serve two different APIs that behave differently:

  • The Connection interface contains several methods that return the current state of the connection or of the last statement. The chosen pattern for that API is to throw an exception if the Connection is not in a valid state for the method to be called. I don't think we should deviate from that pattern for one method. This also somewhat reflects the standard in JDBC, where most methods also throw an exception if the Connection is not in a valid state for the method. See for example the commit() method. (Although this is not 100% true in all cases, as some other JDBC methods have a different contract, and allows you to call it in what seems to be an illogical state. Calling setAutoCommit is for example allowed during a transaction, and will automatically commit the transaction if the auto commit mode).
  • The Connection API also contains a simple SQL parser that can be used to get and set the state of a connection. This 'API' is lenient, and will return null when a value is requested that is not available at that moment. This SQL parser uses the getCommitResponseOrNull and other similar methods.

ConnectionPreconditions.checkState(!isClosed(), CLOSED_ERROR_MSG);
return this.currentUnitOfWork == null ? null : this.currentUnitOfWork.getCommitResponseOrNull();
}

@Override
public void setReturnCommitStats(boolean returnCommitStats) {
ConnectionPreconditions.checkState(!isClosed(), CLOSED_ERROR_MSG);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it useful to check the connection state here? What breaks if this field is set or read on a closed connection?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This also follows the standard of the other methods in this class, as well as the default behavior in JDBC, where all methods throw an exception if it is called on a closed connection. See for example getAutoCommit().

this.returnCommitStats = returnCommitStats;
}

@Override
public boolean isReturnCommitStats() {
ConnectionPreconditions.checkState(!isClosed(), CLOSED_ERROR_MSG);
return this.returnCommitStats;
}

/** Resets this connection to its default transaction options. */
private void setDefaultTransactionOptions() {
if (transactionStack.isEmpty()) {
Expand Down Expand Up @@ -954,6 +983,7 @@ private UnitOfWork createNewUnitOfWork() {
.setReadOnly(isReadOnly())
.setReadOnlyStaleness(readOnlyStaleness)
.setAutocommitDmlMode(autocommitDmlMode)
.setReturnCommitStats(returnCommitStats)
.setStatementTimeout(statementTimeout)
.withStatementExecutor(statementExecutor)
.build();
Expand All @@ -970,6 +1000,7 @@ private UnitOfWork createNewUnitOfWork() {
return ReadWriteTransaction.newBuilder()
.setDatabaseClient(dbClient)
.setRetryAbortsInternally(retryAbortsInternally)
.setReturnCommitStats(returnCommitStats)
.setTransactionRetryListeners(transactionRetryListeners)
.setStatementTimeout(statementTimeout)
.withStatementExecutor(statementExecutor)
Expand Down
Expand Up @@ -155,6 +155,7 @@ public String[] getValidValues() {
private static final String DEFAULT_NUM_CHANNELS = null;
private static final String DEFAULT_USER_AGENT = null;
private static final String DEFAULT_OPTIMIZER_VERSION = "";
private static final boolean DEFAULT_RETURN_COMMIT_STATS = false;
private static final boolean DEFAULT_LENIENT = false;

private static final String PLAIN_TEXT_PROTOCOL = "http:";
Expand Down Expand Up @@ -229,6 +230,7 @@ public String[] getValidValues() {
ConnectionProperty.createStringProperty(
OPTIMIZER_VERSION_PROPERTY_NAME,
"Sets the default query optimizer version to use for this connection."),
ConnectionProperty.createBooleanProperty("returnCommitStats", "", false),
ConnectionProperty.createBooleanProperty(
LENIENT_PROPERTY_NAME,
"Silently ignore unknown properties in the connection string/properties (true/false)",
Expand Down Expand Up @@ -456,6 +458,7 @@ public static Builder newBuilder() {
private final Integer maxSessions;
private final String userAgent;
private final QueryOptions queryOptions;
private final boolean returnCommitStats;

private final boolean autocommit;
private final boolean readOnly;
Expand Down Expand Up @@ -485,6 +488,7 @@ private ConnectionOptions(Builder builder) {
QueryOptions.Builder queryOptionsBuilder = QueryOptions.newBuilder();
queryOptionsBuilder.setOptimizerVersion(parseOptimizerVersion(this.uri));
this.queryOptions = queryOptionsBuilder.build();
this.returnCommitStats = parseReturnCommitStats(this.uri);

this.host =
matcher.group(Builder.HOST_GROUP) == null
Expand Down Expand Up @@ -634,6 +638,12 @@ static String parseOptimizerVersion(String uri) {
return value != null ? value : DEFAULT_OPTIMIZER_VERSION;
}

@VisibleForTesting
static boolean parseReturnCommitStats(String uri) {
String value = parseUriProperty(uri, "returnCommitStats");
return value != null ? Boolean.valueOf(value) : false;
}

@VisibleForTesting
static boolean parseLenient(String uri) {
String value = parseUriProperty(uri, LENIENT_PROPERTY_NAME);
Expand Down Expand Up @@ -823,6 +833,11 @@ QueryOptions getQueryOptions() {
return queryOptions;
}

/** Whether connections created by this {@link ConnectionOptions} return commit stats. */
public boolean isReturnCommitStats() {
return returnCommitStats;
}

/** Interceptors that should be executed after each statement */
List<StatementExecutionInterceptor> getStatementExecutionInterceptors() {
return statementExecutionInterceptors;
Expand Down
Expand Up @@ -56,6 +56,8 @@ interface ConnectionStatementExecutor {

StatementResult statementShowCommitTimestamp();

StatementResult statementShowCommitResponse();

StatementResult statementSetReadOnlyStaleness(TimestampBound staleness);

StatementResult statementShowReadOnlyStaleness();
Expand All @@ -64,6 +66,10 @@ interface ConnectionStatementExecutor {

StatementResult statementShowOptimizerVersion();

StatementResult statementSetReturnCommitStats(Boolean returnCommitStats);

StatementResult statementShowReturnCommitStats();

StatementResult statementBeginTransaction();

StatementResult statementCommit();
Expand Down
Expand Up @@ -27,26 +27,37 @@
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.SET_READONLY;
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.SET_READ_ONLY_STALENESS;
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.SET_RETRY_ABORTS_INTERNALLY;
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.SET_RETURN_COMMIT_STATS;
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.SET_STATEMENT_TIMEOUT;
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.SET_TRANSACTION_MODE;
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.SHOW_AUTOCOMMIT;
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.SHOW_AUTOCOMMIT_DML_MODE;
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.SHOW_COMMIT_RESPONSE;
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.SHOW_COMMIT_TIMESTAMP;
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.SHOW_OPTIMIZER_VERSION;
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.SHOW_READONLY;
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.SHOW_READ_ONLY_STALENESS;
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.SHOW_READ_TIMESTAMP;
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.SHOW_RETRY_ABORTS_INTERNALLY;
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.SHOW_RETURN_COMMIT_STATS;
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.SHOW_STATEMENT_TIMEOUT;
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.START_BATCH_DDL;
import static com.google.cloud.spanner.connection.StatementResult.ClientSideStatementType.START_BATCH_DML;
import static com.google.cloud.spanner.connection.StatementResultImpl.noResult;
import static com.google.cloud.spanner.connection.StatementResultImpl.resultSet;

import com.google.cloud.spanner.CommitResponse;
import com.google.cloud.spanner.CommitStats;
import com.google.cloud.spanner.ResultSet;
import com.google.cloud.spanner.ResultSets;
import com.google.cloud.spanner.Struct;
import com.google.cloud.spanner.TimestampBound;
import com.google.cloud.spanner.Type;
import com.google.cloud.spanner.Type.StructField;
import com.google.cloud.spanner.connection.ReadOnlyStalenessUtil.DurationValueGetter;
import com.google.common.base.Preconditions;
import com.google.protobuf.Duration;
import java.util.Arrays;
import java.util.concurrent.TimeUnit;

/**
Expand Down Expand Up @@ -170,6 +181,28 @@ public StatementResult statementShowCommitTimestamp() {
"COMMIT_TIMESTAMP", getConnection().getCommitTimestampOrNull(), SHOW_COMMIT_TIMESTAMP);
}

@Override
public StatementResult statementShowCommitResponse() {
CommitResponse response = getConnection().getCommitResponseOrNull();
CommitStats stats = null;
if (response != null && response.hasCommitStats()) {
stats = response.getCommitStats();
}
ResultSet resultSet =
ResultSets.forRows(
Type.struct(
StructField.of("COMMIT_TIMESTAMP", Type.timestamp()),
StructField.of("MUTATION_COUNT", Type.int64())),
Arrays.asList(
Struct.newBuilder()
.set("COMMIT_TIMESTAMP")
.to(response == null ? null : response.getCommitTimestamp())
.set("MUTATION_COUNT")
.to(stats == null ? null : stats.getMutationCount())
.build()));
return StatementResultImpl.of(resultSet, SHOW_COMMIT_RESPONSE);
}

@Override
public StatementResult statementSetReadOnlyStaleness(TimestampBound staleness) {
getConnection().setReadOnlyStaleness(staleness);
Expand Down Expand Up @@ -197,6 +230,18 @@ public StatementResult statementShowOptimizerVersion() {
"OPTIMIZER_VERSION", getConnection().getOptimizerVersion(), SHOW_OPTIMIZER_VERSION);
}

@Override
public StatementResult statementSetReturnCommitStats(Boolean returnCommitStats) {
getConnection().setReturnCommitStats(returnCommitStats);
return noResult(SET_RETURN_COMMIT_STATS);
}

@Override
public StatementResult statementShowReturnCommitStats() {
return resultSet(
"RETURN_COMMIT_STATS", getConnection().isReturnCommitStats(), SHOW_RETURN_COMMIT_STATS);
}

@Override
public StatementResult statementBeginTransaction() {
getConnection().beginTransaction();
Expand Down
Expand Up @@ -20,6 +20,7 @@
import com.google.api.core.ApiFutures;
import com.google.api.gax.longrunning.OperationFuture;
import com.google.cloud.Timestamp;
import com.google.cloud.spanner.CommitResponse;
import com.google.cloud.spanner.DatabaseClient;
import com.google.cloud.spanner.ErrorCode;
import com.google.cloud.spanner.Mutation;
Expand Down Expand Up @@ -168,6 +169,17 @@ public Timestamp getCommitTimestampOrNull() {
return null;
}

@Override
public CommitResponse getCommitResponse() {
throw SpannerExceptionFactory.newSpannerException(
ErrorCode.FAILED_PRECONDITION, "There is no commit response available for DDL batches.");
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a code smell suggests that perhaps getCommitResponse does not belong in the superclass/interface in the first place since it doesn't apply to all subclasses.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a package private class (extending from a package private interface), so it is not something that client applications will deal with. Client applications only interact with the Connection interface. (And client applications in this case is only intended to be frameworks /drivers that implement a declarative connection-type interface, such as JDBC.)
These declarative style APIs often mean that certain methods are only applicable in certain states, and this is what is also reflected in these classes. The method that client applications will interact with is Connection#getCommitResponse(). This method will delegate the actual response to the at that moment active UnitOfWork on the Connection. The different UnitOfWork implementations will respond based on what they support, which may also be that they do not support it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered returning an empty object or null instead?

If clients really won't see this, fine. However it's not enough that this class is non-public. They could still invoke this method if they get an instance of it, even while only knowing its supertype.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Returning null or an empty object here would only move the logic to the ConnectionImpl class, which should throw an exception if it is not in a valid state to return CommitStats at that moment (for example because a DDL batch is active). This logic is therefore delegated to the concrete implementations of UnitOfWork.

This class and its super types are all package private and or not accessible from outside the library.

}

@Override
public CommitResponse getCommitResponseOrNull() {
olavloite marked this conversation as resolved.
Show resolved Hide resolved
return null;
}

@Override
public ApiFuture<Void> executeDdlAsync(ParsedStatement ddl) {
ConnectionPreconditions.checkState(
Expand Down
Expand Up @@ -21,6 +21,7 @@
import com.google.api.core.ApiFutures;
import com.google.api.core.SettableApiFuture;
import com.google.cloud.Timestamp;
import com.google.cloud.spanner.CommitResponse;
import com.google.cloud.spanner.ErrorCode;
import com.google.cloud.spanner.Mutation;
import com.google.cloud.spanner.Options.QueryOption;
Expand Down Expand Up @@ -119,6 +120,17 @@ public Timestamp getCommitTimestampOrNull() {
return null;
}

@Override
public CommitResponse getCommitResponse() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Doubles my suspicion that this doesn't belong in the superclass.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See above.

throw SpannerExceptionFactory.newSpannerException(
ErrorCode.FAILED_PRECONDITION, "There is no commit response available for DML batches.");
}

@Override
public CommitResponse getCommitResponseOrNull() {
return null;
}

@Override
public ApiFuture<Void> executeDdlAsync(ParsedStatement ddl) {
throw SpannerExceptionFactory.newSpannerException(
Expand Down