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 support for CommitStats #544

Merged
merged 25 commits into from Feb 17, 2021
Merged

feat!: add support for CommitStats #544

merged 25 commits into from Feb 17, 2021

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Oct 24, 2020

Adds support for returning CommitStats from read/write transactions.

Replaces #522

Adds support for returning CommitStats from read/write transactions.
@olavloite olavloite added the do not merge label Oct 24, 2020
@olavloite olavloite requested review from as code owners Oct 24, 2020
@google-cla google-cla bot added the cla: yes label Oct 24, 2020
@codecov
Copy link

@codecov codecov bot commented Oct 24, 2020

Codecov Report

Merging #544 (65f9b89) into master (f9ac29c) will increase coverage by 0.22%.
The diff coverage is 96.03%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #544      +/-   ##
============================================
+ Coverage     85.04%   85.26%   +0.22%     
- Complexity     2583     2616      +33     
============================================
  Files           143      144       +1     
  Lines         14145    14208      +63     
  Branches       1369     1372       +3     
============================================
+ Hits          12030    12115      +85     
+ Misses         1542     1522      -20     
+ Partials        573      571       -2     
Impacted Files Coverage Δ Complexity Δ
...a/com/google/cloud/spanner/TransactionManager.java 100.00% <ø> (ø) 0.00 <0.00> (ø)
...spanner/admin/database/v1/DatabaseAdminClient.java 83.22% <ø> (ø) 100.00 <0.00> (ø)
...spanner/admin/instance/v1/InstanceAdminClient.java 79.14% <ø> (ø) 56.00 <0.00> (ø)
...ava/com/google/cloud/spanner/v1/SpannerClient.java 82.05% <ø> (ø) 63.00 <0.00> (ø)
...m/google/cloud/spanner/TransactionManagerImpl.java 87.50% <66.66%> (-1.39%) 21.00 <1.00> (+1.00) ⬇️
...cloud/spanner/connection/SingleUseTransaction.java 91.35% <66.66%> (+2.40%) 32.00 <2.00> (ø)
...ud/spanner/SessionPoolAsyncTransactionManager.java 87.21% <85.71%> (+1.50%) 14.00 <1.00> (+3.00)
...ain/java/com/google/cloud/spanner/SessionPool.java 89.23% <92.85%> (-0.02%) 73.00 <0.00> (ø)
...om/google/cloud/spanner/TransactionRunnerImpl.java 86.68% <93.75%> (+0.96%) 10.00 <1.00> (+1.00)
...java/com/google/cloud/spanner/AsyncRunnerImpl.java 93.93% <100.00%> (+1.63%) 9.00 <7.00> (+4.00)
... and 18 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 f9ac29c...7d45ace. Read the comment docs.

@product-auto-label product-auto-label bot added the api: spanner label Oct 25, 2020
@olavloite olavloite requested a review from thiagotnunes Oct 25, 2020
Copy link
Contributor

@skuruppu skuruppu left a comment

I'm not quite sure if we need the breaking change here. If it's just because of the interface update, I wouldn't mark it as such. But if there is an actual breaking change where users will have to update their code in order to use the next release, then let me know.

@olavloite olavloite changed the title feat!: add support for CommitStats feat: add support for CommitStats Oct 31, 2020
@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Oct 31, 2020

I'm not quite sure if we need the breaking change here. If it's just because of the interface update, I wouldn't mark it as such. But if there is an actual breaking change where users will have to update their code in order to use the next release, then let me know.

There are no changes in this PR that will break existing code that just uses these interfaces. Binary compatibility with the previous version will be broken, and code that actually implements the DatabaseClient interface would need to be updated.

@generated-files-bot
Copy link

@generated-files-bot generated-files-bot bot commented Nov 6, 2020

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

  • proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/CommitRequest.java
  • proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/CommitRequestOrBuilder.java
  • proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/CommitResponse.java
  • proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/CommitResponseOrBuilder.java
  • proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/SpannerProto.java
  • proto-google-cloud-spanner-v1/src/main/proto/google/spanner/v1/spanner.proto

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

  • proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/CommitRequest.java
  • proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/CommitRequestOrBuilder.java
  • proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/CommitResponse.java
  • proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/CommitResponseOrBuilder.java
  • proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/SpannerProto.java
  • proto-google-cloud-spanner-v1/src/main/proto/google/spanner/v1/spanner.proto

@generated-files-bot
Copy link

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

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

  • proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/CommitRequest.java
  • proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/CommitRequestOrBuilder.java
  • proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/CommitResponse.java
  • proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/CommitResponseOrBuilder.java
  • proto-google-cloud-spanner-v1/src/main/java/com/google/spanner/v1/SpannerProto.java
  • proto-google-cloud-spanner-v1/src/main/proto/google/spanner/v1/spanner.proto

elharo
elharo previously requested changes Jan 21, 2021
@@ -406,4 +406,62 @@
<className>com/google/cloud/spanner/AbstractLazyInitializer</className>
<method>java.lang.Object initialize()</method>
</difference>

<!-- Support for CommitStats added -->
Copy link
Contributor

@elharo elharo Jan 21, 2021

Choose a reason for hiding this comment

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

another one where a major version bump is required

@skuruppu
Copy link
Contributor

@skuruppu skuruppu commented Jan 23, 2021

@olavloite, I just merged in #817 so you can rebase this PR. In light of the discussions about binary incompatibility, please take a look and see if the implementation needs to be changed to avoid a breaking change or if we'll just have to do a semver updgrade.

@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Jan 24, 2021

@olavloite, I just merged in #817 so you can rebase this PR. In light of the discussions about binary incompatibility, please take a look and see if the implementation needs to be changed to avoid a breaking change or if we'll just have to do a semver updgrade.

@skuruppu One possible way to change this into a non-breaking change would be to return the CommitStats through the options object instead of through the TransactionRunner. In the current implementation requesting and getting the CommitStats looks like this:

    TransactionRunner runner = client.readWriteTransaction(Options.commitStats());
    runner.run(
        new TransactionCallable<Void>() {
          @Override
          public Void run(TransactionContext transaction) throws Exception {
            transaction.buffer(Mutation.delete("FOO", Key.of("foo")));
            return null;
          }
        });
    System.out.println(runner.getCommitResponse().getCommitStats());

That could be changed to the following:

    CommitStatsOption option = Options.commitStats();
    TransactionRunner runner = client.readWriteTransaction(option);
    runner.run(
        new TransactionCallable<Void>() {
          @Override
          public Void run(TransactionContext transaction) throws Exception {
            transaction.buffer(Mutation.delete("FOO", Key.of("foo")));
            return null;
          }
        });
    System.out.println(option.getCommitStats());

In the latter case we don't need to add the new method TransactionRunner#getCommitResponse which is the source of the breaking change in this case. Instead, we make the CommitStatsOption class public (it is currently package-private) and add a getCommitStats() method to it.

WDYT?

@skuruppu
Copy link
Contributor

@skuruppu skuruppu commented Jan 27, 2021

@olavloite, I just merged in #817 so you can rebase this PR. In light of the discussions about binary incompatibility, please take a look and see if the implementation needs to be changed to avoid a breaking change or if we'll just have to do a semver updgrade.

@skuruppu One possible way to change this into a non-breaking change would be to return the CommitStats through the options object instead of through the TransactionRunner. In the current implementation requesting and getting the CommitStats looks like this:

    TransactionRunner runner = client.readWriteTransaction(Options.commitStats());
    runner.run(
        new TransactionCallable<Void>() {
          @Override
          public Void run(TransactionContext transaction) throws Exception {
            transaction.buffer(Mutation.delete("FOO", Key.of("foo")));
            return null;
          }
        });
    System.out.println(runner.getCommitResponse().getCommitStats());

That could be changed to the following:

    CommitStatsOption option = Options.commitStats();
    TransactionRunner runner = client.readWriteTransaction(option);
    runner.run(
        new TransactionCallable<Void>() {
          @Override
          public Void run(TransactionContext transaction) throws Exception {
            transaction.buffer(Mutation.delete("FOO", Key.of("foo")));
            return null;
          }
        });
    System.out.println(option.getCommitStats());

In the latter case we don't need to add the new method TransactionRunner#getCommitResponse which is the source of the breaking change in this case. Instead, we make the CommitStatsOption class public (it is currently package-private) and add a getCommitStats() method to it.

WDYT?

I think that's a great idea. It is an option after all and as long as it's accessible when required, then it works. @thiagotnunes FYI.

@thiagotnunes
Copy link
Contributor

@thiagotnunes thiagotnunes commented Jan 27, 2021

@olavloite hey Knut, after talking with @skuruppu we have decided not to change the design right now, but to do a major release instead.

Copy link
Contributor

@elharo elharo left a comment

It's always acceptable to bump the major version. I think you'll want to add a ! after feat in the PR description (i.e. feat! ) so the release automation notices this. It might also be useful to update the snapshot version in this PR.

After this release goes out, you can remove the ignored differences file and point it at the newly released major version.

@thiagotnunes thiagotnunes changed the title feat: add support for CommitStats feat!: add support for CommitStats Jan 29, 2021
@thiagotnunes
Copy link
Contributor

@thiagotnunes thiagotnunes commented Jan 29, 2021

@elharo Thanks, updated the PR description.


@Test
public void transactionRunnerReturnsCommitStats() {
assumeFalse("Emulator does not return commit statistics", isUsingEmulator());
Copy link
Contributor

@elharo elharo Feb 3, 2021

Choose a reason for hiding this comment

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

The assume here is surprising. Why would this be sometimes be true and sometimes not true in this one test method?

Copy link
Contributor Author

@olavloite olavloite Feb 3, 2021

Choose a reason for hiding this comment

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

isUsingEmulator() checks whether the environment variable SPANNER_EMULATOR_HOST has been set to a non-empty value. If so, the integration tests are running against the emulator. The emulator does not support all features of Cloud Spanner, which means we need to skip some specific tests. The CI environments runs the tests both against the emulator and Cloud Spanner.

@skuruppu skuruppu removed the do not merge label Feb 4, 2021
@skuruppu
Copy link
Contributor

@skuruppu skuruppu commented Feb 4, 2021

@olavloite this can now be merged when you're ready.

@olavloite olavloite requested a review from elharo Feb 4, 2021
@olavloite olavloite added the kokoro:force-run label Feb 4, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Feb 4, 2021
@skuruppu
Copy link
Contributor

@skuruppu skuruppu commented Feb 10, 2021

@elharo friendly ping. This feature is launching soon so we need to start merging in the PRs to make sure that the samples are ready to ingested into the Cloud Docs.

@@ -56,4 +56,10 @@
* {@link ExecutionException} if the transaction did not commit.
*/
ApiFuture<Timestamp> getCommitTimestamp();

/**
* Returns the {@link CommitResponse} of this transaction. {@link ApiFuture#get()} will throw an
Copy link
Contributor

@elharo elharo Feb 16, 2021

Choose a reason for hiding this comment

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

will throw --> throws
per Google style

Copy link
Contributor Author

@olavloite olavloite Feb 16, 2021

Choose a reason for hiding this comment

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

Done

try {
commitTimestamp.set(delegate.getCommitTimestamp());
commitResponse.set(delegate.getCommitResponse());
} catch (Throwable t) {
Copy link
Contributor

@elharo elharo Feb 16, 2021

Choose a reason for hiding this comment

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

letting it slide because it isn't changed in this PR, but catching Throwable is only rarely what you want. This is probably worth filing a bug on.

Copy link
Contributor Author

@olavloite olavloite Feb 16, 2021

Choose a reason for hiding this comment

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

Added issue: #875

@@ -132,29 +133,37 @@ public void onError(Throwable t) {
SpannerExceptionFactory.newSpannerException(
ErrorCode.ABORTED, "Transaction already aborted"));
}
ApiFuture<Timestamp> res = txn.commitAsync();
ApiFuture<CommitResponse> res = txn.commitAsync();
Copy link
Contributor

@elharo elharo Feb 16, 2021

Choose a reason for hiding this comment

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

no abbreviated variable names per google style.
Concretely I did not know what this was when I read it below and had to scroll up to find out.

Copy link
Contributor Author

@olavloite olavloite Feb 16, 2021

Choose a reason for hiding this comment

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

Changed to commitResponseFuture

* row from a parent table that has the ON DELETE CASCADE annotation is also counted as one
* mutation regardless of the number of interleaved child rows present. The exception to this is
* if there are secondary indexes defined on rows being deleted, then the changes to the secondary
* indexes will be counted individually. For example, if a table has 2 secondary indexes, deleting
Copy link
Contributor

@elharo elharo Feb 16, 2021

Choose a reason for hiding this comment

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

will be --> are

Copy link
Contributor Author

@olavloite olavloite Feb 16, 2021

Choose a reason for hiding this comment

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

Done

* mutation regardless of the number of interleaved child rows present. The exception to this is
* if there are secondary indexes defined on rows being deleted, then the changes to the secondary
* indexes will be counted individually. For example, if a table has 2 secondary indexes, deleting
* a range of rows in the table will count as 1 mutation for the table, plus 2 mutations for each
Copy link
Contributor

@elharo elharo Feb 16, 2021

Choose a reason for hiding this comment

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

will count --> counts

Copy link
Contributor Author

@olavloite olavloite Feb 16, 2021

Choose a reason for hiding this comment

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

Done

public void testTransactionOptions_withCommitStatsAndOtherOptionAreNotEqual() {
Options option1 = Options.fromTransactionOptions(Options.commitStats());
Options option2 = Options.fromQueryOptions(Options.prefetchChunks(10));
assertFalse(option1.equals(option2));
Copy link
Contributor

@elharo elharo Feb 16, 2021

Choose a reason for hiding this comment

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

assertNotEquals

Copy link
Contributor Author

@olavloite olavloite Feb 16, 2021

Choose a reason for hiding this comment

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

Done

public void testTransactionOptions_withCommitStatsAreEqual() {
Options option1 = Options.fromTransactionOptions(Options.commitStats());
Options option2 = Options.fromTransactionOptions(Options.commitStats());
assertTrue(option1.equals(option2));
Copy link
Contributor

@elharo elharo Feb 16, 2021

Choose a reason for hiding this comment

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

assertEquals

Copy link
Contributor Author

@olavloite olavloite Feb 16, 2021

Choose a reason for hiding this comment

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

Done

public void testTransactionOptions_noOptionsAreEqual() {
Options option1 = Options.fromTransactionOptions();
Options option2 = Options.fromTransactionOptions();
assertTrue(option1.equals(option2));
Copy link
Contributor

@elharo elharo Feb 16, 2021

Choose a reason for hiding this comment

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

assertEquals

Copy link
Contributor Author

@olavloite olavloite Feb 16, 2021

Choose a reason for hiding this comment

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

Done

assumeFalse("Emulator does not return commit statistics", isUsingEmulator());
try (TransactionManager manager = client.transactionManager(Options.commitStats())) {
TransactionContext transaction = manager.begin();
while (true) {
Copy link
Contributor

@elharo elharo Feb 16, 2021

Choose a reason for hiding this comment

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

This one scares me since it looks like an infinite loop Could you add a counter with a maximum number of retries before failure?

Copy link
Contributor Author

@olavloite olavloite Feb 16, 2021

Choose a reason for hiding this comment

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

This is the recommended way to use TransactionManager according to the documentation. It is also the way it is already used in multiple other test cases. I would rather either:

  1. Keep this and all other instances as they are.
  2. Change this and other instances + the documentation in a separate PR.

assertEquals(2L, manager.getCommitResponse().getCommitStats().getMutationCount());
break;
} catch (AbortedException e) {
Thread.sleep(e.getRetryDelayInMillis() / 1000);
Copy link
Contributor

@elharo elharo Feb 16, 2021

Choose a reason for hiding this comment

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

BUG! Thread.sleep takes milliseconds. No need to divide by 1000.

Copy link
Contributor Author

@olavloite olavloite Feb 16, 2021

Choose a reason for hiding this comment

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

Good point. Changed and filed a bug for the documentation that also includes this.

@elharo elharo dismissed their stale review Feb 16, 2021

major semver bump planned

@thiagotnunes thiagotnunes merged commit 44aa384 into master Feb 17, 2021
17 checks passed
@thiagotnunes thiagotnunes deleted the commit-stats2 branch Feb 17, 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

5 participants