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
Conversation
Adds support for returning CommitStats from read/write transactions.
Co-authored-by: skuruppu <skuruppu@google.com>
google-cloud-spanner/pom.xml
Outdated
@@ -377,5 +377,38 @@ | |||
</plugins> | |||
</build> | |||
</profile> | |||
<profile> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 Report
@@ 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 Continue to review full report at Codecov.
|
There was a problem hiding this 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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
google-cloud-spanner/pom.xml
Outdated
@@ -377,5 +377,38 @@ | |||
</plugins> | |||
</build> | |||
</profile> | |||
<profile> |
There was a problem hiding this comment.
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.
Warning: This pull request is touching the following templated files:
|
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. ℹ️ Googlers: Go here for more info. |
@skuruppu I assume it is OK to merge this one as well once it is approved? |
@@ -41,6 +41,11 @@ public Timestamp getCommitTimestamp() { | |||
return Timestamp.fromProto(proto.getCommitTimestamp()); | |||
} | |||
|
|||
/** @return true if the {@link CommitResponse} includes {@link CommitStats}. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: no period
There was a problem hiding this comment.
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. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: no period
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will throw --> throws
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 theConnection
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 theConnection
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 thegetCommitResponseOrNull
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))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assertTrue
There was a problem hiding this comment.
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))); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
res --> result
There was a problem hiding this comment.
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))); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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))); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
Yes absolutely, please merge once you have all approvals. |
There was a problem hiding this 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:
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 |
@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 |
@@ -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"; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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} |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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."); |
There was a problem hiding this comment.
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."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not been committed
There was a problem hiding this comment.
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."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not been committed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
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
🤖 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).
Adds support for
CommitStats
to the Connection API.