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
Collaborator

Adds support for CommitStats to the Connection API.

@olavloite olavloite requested review from a team as code owners November 6, 2020 19:34
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 6, 2020
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/java-spanner API. label Nov 6, 2020
@@ -377,5 +377,38 @@
</plugins>
</build>
</profile>
<profile>
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 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

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

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

Choose a reason for hiding this comment

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

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

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

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

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 Indicates a pull request not ready for merge, due to either quality or timing. label Nov 9, 2020
@olavloite olavloite requested a review from a team December 5, 2020 09:36
@generated-files-bot
Copy link

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 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 This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Dec 5, 2020
@olavloite
Copy link
Collaborator Author

@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

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

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

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

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

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

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

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

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

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.


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

Choose a reason for hiding this comment

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

assertTrue

Copy link
Collaborator Author

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

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

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

Choose a reason for hiding this comment

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

res --> result

Copy link
Collaborator Author

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

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

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

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 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 Indicates a pull request not ready for merge, due to either quality or timing. label Feb 5, 2021
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

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

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 February 17, 2021 03:20
@skuruppu
Copy link
Contributor

@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
Collaborator Author

@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.

@@ -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

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

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

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

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

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

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

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

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

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 February 24, 2021 14:13

major issues addressed

@skuruppu skuruppu merged commit b2b1191 into master Feb 24, 2021
@skuruppu skuruppu deleted the connection-api-commit-stats branch February 24, 2021 22:49
ansh0l pushed a commit to ansh0l/java-spanner that referenced this pull request Nov 10, 2022
This is an auto-generated regeneration of the .pb.go files by
cloud.google.com/go/internal/gapicgen. Once this PR is submitted, genbot will
update the corresponding PR to depend on the newer version of go-genproto, and
assign reviewers. Whilst this or any regen PR is open in go-genproto, genbot
will not create any more regeneration PRs. If all regen PRs are closed,
gapicgen will create a new set of regeneration PRs once per night.

If you have been assigned to review this PR, please:

- Ensure that CI is passing. If it's failing, it requires your manual attention.
- Approve and submit this PR if you believe it's ready to ship. That will prompt
genbot to assign reviewers to the google-cloud-go PR.

Corresponding google-cloud-go PR: googleapis/google-cloud-go#4193

Changes:

chore: regenerate API index

  Source-Link: googleapis/googleapis@0b26f05

fix!: Specify namespace options for C# and PHP.
  This is only a breaking change for C# and PHP, which haven't published client libraries for this API yet.

  PiperOrigin-RevId: 376845575
  Source-Link: googleapis/googleapis@5908eff
rajatbhatta pushed a commit to rajatbhatta/java-spanner that referenced this pull request Nov 17, 2022
🤖 I have created a release \*beep\* \*boop\*
---
### [2.4.3](https://www.github.com/googleapis/java-spanner-jdbc/compare/v2.4.2...v2.4.3) (2021-09-14)


### Dependencies

* update dependency com.google.cloud:google-cloud-shared-dependencies to v2.2.1 ([googleapis#606](https://www.github.com/googleapis/java-spanner-jdbc/issues/606)) ([36c1791](https://www.github.com/googleapis/java-spanner-jdbc/commit/36c17916e2891d6c13ea6437a328dae8e16ffc13))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/java-spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants