Skip to content

Conversation

@mayurkale22
Copy link
Member

Adds a sample for assigning and querying tags.

Follow-up to #576

Note: We will add the test when the feature is fully available.

@mayurkale22 mayurkale22 requested a review from a team as a code owner April 7, 2021 18:17
@mayurkale22 mayurkale22 requested a review from a team April 7, 2021 18:17
@snippet-bot
Copy link

snippet-bot bot commented Apr 7, 2021

Here is the summary of changes.

You are about to add 2 region tags.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/java-spanner API. label Apr 7, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Apr 7, 2021
@product-auto-label product-auto-label bot added the samples Issues that are directly related to samples. label Apr 7, 2021
@mayurkale22 mayurkale22 requested a review from skuruppu April 7, 2021 18:18
@codecov
Copy link

codecov bot commented Apr 7, 2021

Codecov Report

Merging #1042 (a558647) into master (0bc9972) will increase coverage by 0.05%.
The diff coverage is n/a.

❗ Current head a558647 differs from pull request most recent head 9ad4e41. Consider uploading reports for the commit 9ad4e41 to get more accurate results
Impacted file tree graph

@@ Coverage Diff @@ ## master #1042 +/- ## ============================================ + Coverage 85.18% 85.24% +0.05%  - Complexity 2640 2658 +18  ============================================ Files 155 155 Lines 14413 14453 +40 Branches 1348 1362 +14 ============================================ + Hits 12278 12320 +42  + Misses 1567 1565 -2  Partials 568 568 
Impacted Files Coverage Δ
...oud/spanner/connection/LocalConnectionChecker.java 81.57% <0.00%> (-0.48%) ⬇️
...ain/java/com/google/cloud/spanner/SessionPool.java 88.73% <0.00%> (-0.39%) ⬇️
.../com/google/cloud/spanner/AbstractReadContext.java 87.02% <0.00%> (+0.19%) ⬆️
...om/google/cloud/spanner/TransactionRunnerImpl.java 86.51% <0.00%> (+0.27%) ⬆️
...ain/java/com/google/cloud/spanner/SessionImpl.java 87.20% <0.00%> (+0.30%) ⬆️
...ogle/cloud/spanner/connection/StatementParser.java 87.50% <0.00%> (+0.33%) ⬆️
...rc/main/java/com/google/cloud/spanner/Options.java 92.85% <0.00%> (+0.59%) ⬆️
...oogle/cloud/spanner/PartitionedDmlTransaction.java 83.33% <0.00%> (+0.72%) ⬆️
...ud/spanner/SessionPoolAsyncTransactionManager.java 87.21% <0.00%> (+1.50%) ⬆️
...m/google/cloud/spanner/connection/SpannerPool.java 89.47% <0.00%> (+1.57%) ⬆️
... and 1 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 0bc9972...9ad4e41. Read the comment docs.

@thiagotnunes thiagotnunes requested a review from olavloite April 7, 2021 23:15
@skuruppu skuruppu requested a review from thiagotnunes April 7, 2021 23:52
Copy link
Contributor

@thiagotnunes thiagotnunes left a comment

Choose a reason for hiding this comment

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

It seems that the first sample spanner_set_transaction_and_request_tags does not entirely match the one we have in the client spec.

Is that true, or did we decide to change directions @larkee ?

// Execute query on Query statistics
// see: https://cloud.google.com/spanner/docs/introspection/query-statistics, for more details.
String sql =
"SELECT t.REQUEST_TAG, t.AVG_LATENCY_SECONDS, t.AVG_CPU_SECONDS "
Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I can tell, REQUEST_TAG is not yet available in (public) Cloud Spanner databases. This should therefore not be merged until the column is available.

Furthermore: The t. prefix here does not work. There's no alias t in the query below.

@larkee larkee added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 8, 2021
@mayurkale22
Copy link
Member Author

It seems that the first sample spanner_set_transaction_and_request_tags does not entirely match the one we have in the client spec.

Is that true, or did we decide to change directions @larkee ?

Updated the sample wrt the client spec. PTAL

@thiagotnunes
Copy link
Contributor

@mayurkale22 could you add some tests to this sample? We would like to make sure everything is working correctly before merging it.

@mayurkale22
Copy link
Member Author

@mayurkale22 could you add some tests to this sample? We would like to make sure everything is working correctly before merging it.

Sure, I will add test soon. I will wait until the new column becomes visible for tests.

@eaball35 eaball35 removed their assignment Sep 20, 2021
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. do not merge Indicates a pull request not ready for merge, due to either quality or timing. samples Issues that are directly related to samples.

6 participants