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

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Nov 6, 2020

Adds support for CommitStats to the Connection API.

@olavloite olavloite requested a review from thiagotnunes Nov 6, 2020
@olavloite olavloite requested review from as code owners Nov 6, 2020
@google-cla google-cla bot added the cla: yes label Nov 6, 2020
@product-auto-label product-auto-label bot added the api: spanner label Nov 6, 2020
@@ -377,5 +377,38 @@
</plugins>
</build>
</profile>
<profile>
Copy link
Contributor Author

@olavloite olavloite Nov 6, 2020

Choose a reason for hiding this comment

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

This was a missing piece of configuration that was still in the JDBC repository, but that belongs in the Connection API. It is a Maven profile for automatically generating tests for new client side SQL statements.

Copy link
Contributor

@thiagotnunes thiagotnunes Nov 8, 2020

Choose a reason for hiding this comment

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

Could we add this as a separate PR? Otherwise we won't have this until the commit stats is merged into master.

Copy link
Contributor Author

@olavloite olavloite Nov 9, 2020

Choose a reason for hiding this comment

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

Good point, I've added #611 for that, and I'll remove it from this PR.

@codecov
Copy link

@codecov codecov bot commented Nov 6, 2020

Codecov Report

Merging #608 (8a4c95c) into master (7f4ccf2) will increase coverage by 0.01%.
The diff coverage is 91.95%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #608      +/-   ##
============================================
+ Coverage     85.21%   85.22%   +0.01%     
- Complexity     2624     2653      +29     
============================================
  Files           145      145              
  Lines         14287    14358      +71     
  Branches       1379     1391      +12     
============================================
+ Hits          12174    12237      +63     
- Misses         1538     1539       +1     
- Partials        575      582       +7     
Impacted Files Coverage Δ Complexity Δ
...om/google/cloud/spanner/connection/Connection.java 100.00% <ø> (ø) 0.00 <0.00> (ø)
...om/google/cloud/spanner/connection/UnitOfWork.java 78.57% <ø> (ø) 0.00 <0.00> (ø)
...oogle/cloud/spanner/connection/ConnectionImpl.java 84.37% <64.28%> (-0.57%) 180.00 <5.00> (+5.00) ⬇️
...cloud/spanner/connection/SingleUseTransaction.java 91.32% <94.44%> (-0.03%) 36.00 <6.00> (+4.00) ⬇️
...er/connection/ConnectionStatementExecutorImpl.java 98.85% <95.23%> (-1.15%) 34.00 <5.00> (+5.00) ⬇️
.../java/com/google/cloud/spanner/CommitResponse.java 90.47% <100.00%> (+0.47%) 9.00 <1.00> (+1.00)
...le/cloud/spanner/connection/ConnectionOptions.java 90.21% <100.00%> (+0.21%) 82.00 <3.00> (+3.00)
.../com/google/cloud/spanner/connection/DdlBatch.java 90.10% <100.00%> (+0.22%) 29.00 <2.00> (+2.00)
.../com/google/cloud/spanner/connection/DmlBatch.java 83.01% <100.00%> (+0.66%) 18.00 <2.00> (+2.00)
.../cloud/spanner/connection/ReadOnlyTransaction.java 86.95% <100.00%> (+0.59%) 24.00 <2.00> (+2.00)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f4ccf2...f86255e. Read the comment docs.

Copy link
Contributor

@thiagotnunes thiagotnunes left a comment

LGTM, just a few questions

@@ -1931,9148 +1931,11005 @@ NEW_CONNECTION;
@EXPECT EXCEPTION INVALID_ARGUMENT
show variable/-read_only_staleness;
NEW_CONNECTION;
begin;
show variable optimizer_version;
Copy link
Contributor

@thiagotnunes thiagotnunes Nov 8, 2020

Choose a reason for hiding this comment

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

Is this part of this PR? Maybe we forgot to do something in the query stats (optimizer_version) feature?

Copy link
Contributor Author

@olavloite olavloite Nov 9, 2020

Choose a reason for hiding this comment

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

I forgot to regenerate the test sql files for the optimizer_version feature, so that's why that is included here. The generator does not support generating tests for only some features, so it's impossible to keep that change separate from the changes for CommitStats without completely rewriting the generator.

@@ -377,5 +377,38 @@
</plugins>
</build>
</profile>
<profile>
Copy link
Contributor

@thiagotnunes thiagotnunes Nov 8, 2020

Choose a reason for hiding this comment

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

Could we add this as a separate PR? Otherwise we won't have this until the commit stats is merged into master.

@thiagotnunes thiagotnunes added the do not merge label Nov 9, 2020
@olavloite olavloite requested a review from Dec 5, 2020
@generated-files-bot
Copy link

@generated-files-bot generated-files-bot bot commented Dec 5, 2020

Warning: This pull request is touching the following templated files:

  • .github/blunderbuss.yml
  • .github/workflows/approve-readme.yaml
  • .github/workflows/auto-release.yaml
  • .github/workflows/formatting.yaml
  • .kokoro/common.sh
  • .kokoro/readme.sh
  • .kokoro/release/stage.sh
  • proto-google-cloud-spanner-admin-database-v1/src/main/java/com/google/spanner/admin/database/v1/SpannerDatabaseAdminProto.java
  • proto-google-cloud-spanner-admin-database-v1/src/main/java/com/google/spanner/admin/database/v1/UpdateDatabaseDdlMetadata.java
  • proto-google-cloud-spanner-admin-database-v1/src/main/java/com/google/spanner/admin/database/v1/UpdateDatabaseDdlMetadataOrBuilder.java
  • proto-google-cloud-spanner-admin-database-v1/src/main/proto/google/spanner/admin/database/v1/spanner_database_admin.proto

@google-cla
Copy link

@google-cla google-cla bot commented Dec 5, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Dec 5, 2020
@olavloite olavloite requested a review from elharo Feb 4, 2021
@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Feb 4, 2021

@skuruppu I assume it is OK to merge this one as well once it is approved?

elharo
elharo previously requested changes Feb 4, 2021
@@ -41,6 +41,11 @@ public Timestamp getCommitTimestamp() {
return Timestamp.fromProto(proto.getCommitTimestamp());
}

/** @return true if the {@link CommitResponse} includes {@link CommitStats}. */
Copy link
Contributor

@elharo elharo Feb 4, 2021

Choose a reason for hiding this comment

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

nit: no period

Copy link
Contributor Author

@olavloite olavloite Feb 4, 2021

Choose a reason for hiding this comment

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

Done.

*/
void setReturnCommitStats(boolean returnCommitStats);

/** @return true if this connection requests commit statistics from Cloud Spanner. */
Copy link
Contributor

@elharo elharo Feb 4, 2021

Choose a reason for hiding this comment

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

nit: no period

Copy link
Contributor Author

@olavloite olavloite Feb 4, 2021

Choose a reason for hiding this comment

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

Done.

@@ -632,6 +642,17 @@
*/
Timestamp getCommitTimestamp();

/**
* @return the {@link CommitResponse} of the last {@link TransactionMode#READ_WRITE_TRANSACTION}
* transaction. This method will throw a {@link SpannerException} if there is no last {@link
Copy link
Contributor

@elharo elharo Feb 4, 2021

Choose a reason for hiding this comment

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

will throw --> throws

Copy link
Contributor Author

@olavloite olavloite Feb 4, 2021

Choose a reason for hiding this comment

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

Done.

/**
* @return the {@link CommitResponse} 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
Copy link
Contributor

@elharo elharo Feb 4, 2021

Choose a reason for hiding this comment

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

per google style avoid Latin i.e. Perhaps rewrite as

TransactionMode#READ_WRITE_TRANSACTION} transaction. That is, if the last transaction was a {@link

Copy link
Contributor Author

@olavloite olavloite Feb 4, 2021

Choose a reason for hiding this comment

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

Done.

return this.currentUnitOfWork.getCommitResponse();
}

CommitResponse getCommitResponseOrNull() {
Copy link
Contributor

@elharo elharo Feb 4, 2021

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
Contributor Author

@olavloite olavloite Feb 4, 2021

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.


StatementResult res = subject.execute(Statement.of("set return_commit_stats=true"));
assertThat(res.getResultType(), is(equalTo(ResultType.NO_RESULT)));
assertThat(subject.isReturnCommitStats(), is(equalTo(true)));
Copy link
Contributor

@elharo elharo Feb 4, 2021

Choose a reason for hiding this comment

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

assertTrue

Copy link
Contributor Author

@olavloite olavloite Feb 4, 2021

Choose a reason for hiding this comment

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

Done.


res = subject.execute(Statement.of("set return_commit_stats=false"));
assertThat(res.getResultType(), is(equalTo(ResultType.NO_RESULT)));
assertThat(subject.isReturnCommitStats(), is(equalTo(false)));
Copy link
Contributor

@elharo elharo Feb 4, 2021

Choose a reason for hiding this comment

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

assertFalse is simpler and easier to read

Copy link
Contributor Author

@olavloite olavloite Feb 4, 2021

Choose a reason for hiding this comment

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

Done.

.build())) {
assertThat(subject.isReturnCommitStats(), is(equalTo(false)));

StatementResult res = subject.execute(Statement.of("set return_commit_stats=true"));
Copy link
Contributor

@elharo elharo Feb 4, 2021

Choose a reason for hiding this comment

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

res --> result

Copy link
Contributor Author

@olavloite olavloite Feb 4, 2021

Choose a reason for hiding this comment

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

Done.

.setCredentials(NoCredentials.getInstance())
.setUri(URI)
.build())) {
assertThat(subject.isReturnCommitStats(), is(equalTo(false)));
Copy link
Contributor

@elharo elharo Feb 4, 2021

Choose a reason for hiding this comment

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

assertFalse is simpler and easier to read

Copy link
Contributor Author

@olavloite olavloite Feb 4, 2021

Choose a reason for hiding this comment

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

Done.

subject.execute(Statement.of("set return_commit_stats=yes"));
fail("Missing expected exception");
} catch (SpannerException e) {
assertThat(e.getErrorCode(), is(equalTo(ErrorCode.INVALID_ARGUMENT)));
Copy link
Contributor

@elharo elharo Feb 4, 2021

Choose a reason for hiding this comment

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

If you muse use Truth, then the way to write this is

assertThat(e.getErrorCode()).isEqualTo(ErrorCode.INVALID_ARGUMENT)

but frankly you're better off not even using truth for basic equality, same as, null, non-null, and true and false comparisons. Truth is better reserved for more complex comparisons that JUnit doesn't natively support.

Copy link
Contributor Author

@olavloite olavloite Feb 4, 2021

Choose a reason for hiding this comment

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

This is actually not Truth, but Hamcrest. That was what this entire client library initially used, and most classes have gradually been migrated to Truth. This class has not.

I'll stick to JUnit for the simpler comparisons from now on.

(And changed this specific one to JUnit)

@skuruppu
Copy link
Contributor

@skuruppu skuruppu commented Feb 4, 2021

@skuruppu I assume it is OK to merge this one as well once it is approved?

Yes absolutely, please merge once you have all approvals.

@olavloite olavloite removed the do not merge label Feb 5, 2021
Copy link
Contributor

@elharo elharo left a comment

Looks like you need to manually merge and resolve this one.

releaseplease is not yet signalling a major version update:

#850

I'm checking how that works.

@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Feb 16, 2021

Looks like you need to manually merge and resolve this one.

releaseplease is not yet signalling a major version update:

#850

I'm checking how that works.

I'll look into the merge conflict right away.

Could it be that the issue with releaseplease is that this PR is currently based on the branch commit-stats2 and not master, as it depends on the changes that are made in the client library for CommitStats?

Base automatically changed from commit-stats2 to master Feb 17, 2021
@skuruppu
Copy link
Contributor

@skuruppu skuruppu commented Feb 23, 2021

@olavloite @elharo is there any update on this? The feature is getting launched in a couple of days so it will be really good to have this merged so that the JDBC driver change can be merged asap.

I guess we're going to run into an issue with the BOM dependency.

@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Feb 23, 2021

@olavloite @elharo is there any update on this? The feature is getting launched in a couple of days so it will be really good to have this merged so that the JDBC driver change can be merged asap.

I guess we're going to run into an issue with the BOM dependency.

I've resolved all conflicts and updated the PR.

A note regarding the breaking changes: This PR adds three methods to the com.google.cloud.spanner.connection.Connection interface. That interface (and the entire package where it is located) is marked with @InternalApi and is noted in the documentation that it may make breaking changes without prior notice.

@olavloite olavloite requested a review from elharo Feb 23, 2021
@@ -184,6 +185,8 @@ public String getDefaultValue() {
private static final String USER_AGENT_PROPERTY_NAME = "userAgent";
/** Query optimizer version to use for a connection. */
private static final String OPTIMIZER_VERSION_PROPERTY_NAME = "optimizerVersion";
/** Query optimizer version to use for a connection. */
private static final String RETURN_COMMIT_STATS_PROPERTY_NAME = "returnCommitStats";
Copy link
Contributor

@elharo elharo Feb 24, 2021

Choose a reason for hiding this comment

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

Generally, this is not how Google handles this situation. You don't have to, and probably shouldn't, change the existing code in this PR. However all new code should be as clean as possible, even when that introduces inconsistencies with existing code. If that means one parameter is handled differently than the rest, so be it. Otherwise initial problems simply replicate across the code base over time.

@VisibleForTesting
static boolean parseReturnCommitStats(String uri) {
String value = parseUriProperty(uri, RETURN_COMMIT_STATS_PROPERTY_NAME);
return value != null ? Boolean.valueOf(value) : DEFAULT_RETURN_COMMIT_STATS;
Copy link
Contributor

@elharo elharo Feb 24, 2021

Choose a reason for hiding this comment

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

That sounds like a good case for docs or comments.

@@ -823,6 +836,13 @@ QueryOptions getQueryOptions() {
return queryOptions;
}

/**
* The initial returnCommitStats value for connections created by this {@link ConnectionOptions}
Copy link
Contributor

@elharo elharo Feb 24, 2021

Choose a reason for hiding this comment

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

Only the initial value? If it changes, this still returns the initial value? That's surprising. Hmm, looks like it's final so no need to say initial. probably rewrite as, "whether connections created by this {@link ConnectionOptions} return commit stats"

Copy link
Contributor Author

@olavloite olavloite Feb 24, 2021

Choose a reason for hiding this comment

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

Changed to Whether connections created by this {@link ConnectionOptions} return commit stats

The 'initial' part reflects that fact that while this value is final for the ConnectionOptions, it can still be changed on any Connection that is created from this ConnectionOptions through the Connection#setReturnCommitStats(boolean) method, or by executing the SET RETURN_COMMIT_STATS = TRUE|FALSE sql statement.

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

@elharo elharo Feb 24, 2021

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.

}

@Override
public Timestamp getCommitTimestamp() {
ConnectionPreconditions.checkState(hasCommitTimestamp(), "This transaction has not committed.");
return get(commitTimestampFuture);
ConnectionPreconditions.checkState(hasCommitResponse(), "This transaction has not committed.");
Copy link
Contributor

@elharo elharo Feb 24, 2021

Choose a reason for hiding this comment

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

not been committed

Copy link
Contributor Author

@olavloite olavloite Feb 24, 2021

Choose a reason for hiding this comment

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

done


@Override
public CommitResponse getCommitResponse() {
ConnectionPreconditions.checkState(hasCommitResponse(), "This transaction has not committed.");
Copy link
Contributor

@elharo elharo Feb 24, 2021

Choose a reason for hiding this comment

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

not been committed

Copy link
Contributor Author

@olavloite olavloite Feb 24, 2021

Choose a reason for hiding this comment

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

done

@elharo elharo dismissed their stale review Feb 24, 2021

major issues addressed

@skuruppu skuruppu merged commit b2b1191 into master Feb 24, 2021
17 checks passed
@skuruppu skuruppu deleted the connection-api-commit-stats branch Feb 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants