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 tagging #576
Conversation
This comment has been minimized.
This comment has been minimized.
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 approach looks generally good to me.
google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractReadContext.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractReadContext.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseClientImpl.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseClientImpl.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPool.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/TransactionRunnerImpl.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/TransactionRunnerImpl.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/OptionsTest.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
+@syeduguri Please review |
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 good to me overall
This comment has been minimized.
This comment has been minimized.
google-cloud-spanner/src/main/java/com/google/cloud/spanner/TransactionContext.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## master #576 +/- ##
============================================
+ Coverage 85.18% 85.27% +0.08%
- Complexity 2640 2660 +20
============================================
Files 155 155
Lines 14413 14449 +36
Branches 1348 1362 +14
============================================
+ Hits 12278 12321 +43
+ Misses 1567 1561 -6
+ Partials 568 567 -1 Continue to review full report at Codecov.
|
@olavloite @thiagotnunes I re-rebased PR based on #716, I'd like to get your reviews and approval. |
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 think the transaction tag feature could be simplified somewhat. See my suggestions below.
google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractReadContext.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractReadContext.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractReadContext.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractReadContext.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractReadContext.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/TransactionRunnerImpl.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/TransactionRunnerImpl.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/Options.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/TransactionRunnerImpl.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/AbstractReadContextTest.java
Show resolved
Hide resolved
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.
@olavloite thanks for the suggestions. I made the necessary changes, PTAL.
google-cloud-spanner/src/test/java/com/google/cloud/spanner/AbstractReadContextTest.java
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractReadContext.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/AbstractReadContext.java
Outdated
Show resolved
Hide resolved
@thiagotnunes @elharo @skuruppu this is ready to review. PTAL |
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.
Just a few nits, but feel free to ignore them
* {@link AbstractReadContext} does not have a transaction tag. | ||
*/ | ||
@Nullable | ||
String getTransactionTag() { |
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: could we also create a hasTransactionTag()
that checks if getTransactionTag() != null
?
google-cloud-spanner/src/test/java/com/google/cloud/spanner/OptionsTest.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/OptionsTest.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/OptionsTest.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/OptionsTest.java
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/OptionsTest.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/OptionsTest.java
Show resolved
Hide resolved
The samples build failure is unrelated to this change |
google-cloud-spanner/src/test/java/com/google/cloud/spanner/OptionsTest.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/OptionsTest.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/OptionsTest.java
Show resolved
Hide resolved
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#3943 Changes: feat(cloudbuild/apiv1): Add fields for Pub/Sub triggers Committer: @gleeper PiperOrigin-RevId: 368533270 Source-Link: googleapis/googleapis@9a9e296 fix: configuring timeouts for aiplatform v1 methods PiperOrigin-RevId: 368532697 Source-Link: googleapis/googleapis@a8615e2
🤖 I have created a release \*beep\* \*boop\* --- ## [2.4.0](https://www.github.com/googleapis/java-spanner-jdbc/compare/v2.3.5...v2.4.0) (2021-08-27) ### Features * support JSON data type ([googleapis#447](https://www.github.com/googleapis/java-spanner-jdbc/issues/447)) ([ca1c906](https://www.github.com/googleapis/java-spanner-jdbc/commit/ca1c906e1ed3cad6444068ab9c8465401d6d3074)) ### Dependencies * update dependency com.google.cloud:google-cloud-spanner-bom to v6.12.1 ([googleapis#577](https://www.github.com/googleapis/java-spanner-jdbc/issues/577)) ([a78b177](https://www.github.com/googleapis/java-spanner-jdbc/commit/a78b177f97c298a5b43fcadbca125e957e9f781a)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
Add support to enable users to set
tags
on database operations such as reads, queries and transactions.Background
The statistics of queries, reads and transactions in Cloud Spanner are stored. Providing tags for database operations will allow these statistics to be grouped by a tag and makes queries/transactions easily searchable by tag. This will help make the information provided by the statistics more useful.
TODO
RequestOptions
proto changes are published.Note: samples will be handled in a separate PR.