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
Collaborator

@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 Indicates a pull request not ready for merge, due to either quality or timing. label Oct 24, 2020
@olavloite olavloite requested review from a team as code owners October 24, 2020 10:59
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 24, 2020
@codecov
Copy link

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 Issues related to the googleapis/java-spanner API. label Oct 25, 2020
Copy link
Contributor

@skuruppu skuruppu left a comment

Choose a reason for hiding this comment

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

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

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

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

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

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

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

@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
Copy link
Contributor

@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

@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

Choose a reason for hiding this comment

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

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

@elharo Thanks, updated the PR description.


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

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

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

skuruppu commented Feb 4, 2021

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

@olavloite olavloite requested a review from elharo February 4, 2021 08:06
@olavloite olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 4, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 4, 2021
@skuruppu
Copy link
Contributor

@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

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

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

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

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

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

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

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

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

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

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

Choose a reason for hiding this comment

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

assertNotEquals

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

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

Choose a reason for hiding this comment

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

assertEquals

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

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

Choose a reason for hiding this comment

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

assertEquals

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

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

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

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

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

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 February 16, 2021 15:12

major semver bump planned

@thiagotnunes thiagotnunes merged commit 44aa384 into master Feb 17, 2021
@thiagotnunes thiagotnunes deleted the commit-stats2 branch February 17, 2021 03:20
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#3765

Changes:

build: update package name to already published name
  PiperOrigin-RevId: 360469636
  Source-Link: googleapis/googleapis@c5435cb

feat: allow to disable webhook invocation per request
  PiperOrigin-RevId: 360468675
  Source-Link: googleapis/googleapis@a031936

build(policytroubleshooter): update BUILD file for policy troubleshooter
  PiperOrigin-RevId: 360450845
  Source-Link: googleapis/googleapis@2f0798a

fix(functions): Fix service namespace in grpc_service_config.
  PiperOrigin-RevId: 360443299
  Source-Link: googleapis/googleapis@7cef880

chore(apigateway): Set namespaces for Ruby, PHP, and C# clients
  PiperOrigin-RevId: 360255957
  Source-Link: googleapis/googleapis@7f8b19b
rajatbhatta pushed a commit to rajatbhatta/java-spanner that referenced this pull request Nov 17, 2022
…cies to v2 (googleapis#544)

* deps: update dependency com.google.cloud:google-cloud-shared-dependencies to v2

* Update pom.xml

Co-authored-by: Neenu Shaji <Neenu1995@users.noreply.github.com>
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

5 participants