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 tagging to Connection API #623

Merged
merged 7 commits into from Jul 5, 2021
Merged

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Nov 13, 2020

Adds support for tagging in the Connection API. This is needed to support tagging in the JDBC driver.

This is a binary breaking change, as it adds 4 methods to the Connection interface. That interface is marked as @InternalApi and warns that it may make breaking changes without prior notice.

@olavloite olavloite added the do not merge label Nov 13, 2020
@olavloite olavloite requested a review from thiagotnunes Nov 13, 2020
@olavloite olavloite requested review from as code owners Nov 13, 2020
@google-cla google-cla bot added the cla: yes label Nov 13, 2020
@product-auto-label product-auto-label bot added the api: spanner label Nov 13, 2020
@olavloite olavloite changed the title Connection api tagging feat: add support for tagging to Connection API Nov 13, 2020
Copy link

@yoshi-code-bot yoshi-code-bot left a comment

Some suggestions could not be made:

  • google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/Connection.java
    • lines 38-38

@@ -959,6 +1034,7 @@ private UnitOfWork createNewUnitOfWork() {
.setReadOnlyStaleness(readOnlyStaleness)
.setStatementTimeout(statementTimeout)
.withStatementExecutor(statementExecutor)
.setTransactionTag(transactionTag)
Copy link
Contributor

@thiagotnunes thiagotnunes Nov 16, 2020

Choose a reason for hiding this comment

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

How will this behave if the user never set a transaction tag?

Copy link
Contributor Author

@olavloite olavloite Nov 16, 2020

Choose a reason for hiding this comment

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

It is nullable, so that is not a problem. It is verified by this test case.

}

@Override
public void setStatementTag(String tag) {
Copy link
Contributor

@thiagotnunes thiagotnunes Nov 16, 2020

Choose a reason for hiding this comment

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

Out of curiosity, could we prevent the calling of this for a COMMIT? The user should not be able to do a statement tag in that case.

Copy link
Contributor Author

@olavloite olavloite Nov 16, 2020

Choose a reason for hiding this comment

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

Good question. We can't prevent it before the COMMIT, but we can throw an exception if COMMIT is called after a statement tag has been set, and I think that makes sense as we are quite strict in checking the order of other statements (e.g. you are only allowed to get a commit timestamp if you actually committed etc.).

Copy link
Contributor Author

@olavloite olavloite Nov 16, 2020

Choose a reason for hiding this comment

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

I've changed the implementation to check for this, and to throw an error if the application tries to set a statement tag for a COMMIT or ROLLBACK statement. This change also adds a change relating to DML batches: DML batches can only include one statement tag, as the statements are sent as one ExecuteBatchDmlRequest, which only allows one statement tag. This statement tag must be set before calling START BATCH DML, e.g.

SET STATEMENT_TAG = 'tag-1';
START BATCH DML;
INSERT INTO Singers (SingerId, Name) VALUES (1, 'Morrison');
INSERT INTO Singers (SingerId, Name) VALUES (2, 'Pieterson');
RUN BATCH;

@olavloite olavloite requested a review from thiagotnunes Nov 16, 2020
@olavloite olavloite removed the do not merge label Apr 13, 2021
@codecov
Copy link

@codecov codecov bot commented Apr 13, 2021

Codecov Report

Merging #623 (0868a05) into master (004c5d7) will increase coverage by 0.17%.
The diff coverage is 86.90%.

Current head 0868a05 differs from pull request most recent head df0b11d. Consider uploading reports for the commit df0b11d to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##             master     #623      +/-   ##
============================================
+ Coverage     85.06%   85.23%   +0.17%     
- Complexity     2585     2687     +102     
============================================
  Files           143      155      +12     
  Lines         14130    14531     +401     
  Branches       1368     1376       +8     
============================================
+ Hits          12019    12386     +367     
- Misses         1540     1570      +30     
- Partials        571      575       +4     
Impacted Files Coverage Δ
...a/com/google/cloud/spanner/SessionPoolOptions.java 69.53% <ø> (-0.79%) ⬇️
.../java/com/google/cloud/spanner/SpannerOptions.java 89.50% <ø> (-0.04%) ⬇️
...a/com/google/cloud/spanner/TransactionManager.java 100.00% <ø> (ø)
...spanner/admin/database/v1/DatabaseAdminClient.java 87.96% <ø> (+4.74%) ⬆️
...anner/admin/database/v1/DatabaseAdminSettings.java 15.94% <ø> (ø)
...nner/admin/database/v1/stub/DatabaseAdminStub.java 3.70% <ø> (ø)
...in/database/v1/stub/DatabaseAdminStubSettings.java 93.45% <ø> (-0.10%) ⬇️
...base/v1/stub/GrpcDatabaseAdminCallableFactory.java 50.00% <ø> (ø)
.../admin/database/v1/stub/GrpcDatabaseAdminStub.java 97.60% <ø> (+0.29%) ⬆️
...spanner/admin/instance/v1/InstanceAdminClient.java 82.99% <ø> (+3.85%) ⬆️
... and 122 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 f257671...df0b11d. Read the comment docs.

@googleapis googleapis deleted a comment from generated-files-bot bot Apr 13, 2021
@googleapis googleapis deleted a comment from generated-files-bot bot Apr 13, 2021
@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Apr 13, 2021

@thiagotnunes Would you mind taking a final look before I merge? I've rebased on master and realigned with the last choice that were made for tagging in the client library. Also: This is a binary breaking change as it adds methods to the Connection interface. Do we need a version bump for this?

@thiagotnunes thiagotnunes added the do not merge label Apr 14, 2021
@olavloite olavloite added automerge and removed do not merge labels Jul 5, 2021
@olavloite olavloite merged commit 5722372 into master Jul 5, 2021
18 checks passed
@olavloite olavloite deleted the connection-api-tagging branch Jul 5, 2021
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge label Jul 5, 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

3 participants