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(spanner): support CommitStats #3444

Merged
merged 16 commits into from Feb 4, 2021
Merged

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Dec 10, 2020

Adds support for CommitStats to the Spanner client.

Based on #3056.

@olavloite olavloite added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Dec 10, 2020
@olavloite olavloite requested a review from skuruppu Dec 10, 2020
@olavloite olavloite requested a review from a team as a code owner Dec 10, 2020
@product-auto-label product-auto-label bot added the api: spanner Issues related to the Cloud Spanner API. label Dec 10, 2020
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 10, 2020
@olavloite
Copy link
Contributor Author

olavloite commented Jan 26, 2021

@skuruppu The current build failure seems to be caused by a test project that does not (yet) support CommitStats. Running the integration tests against the Go cloud test project does not return any CommitStats, running the same tests against another dev project does return CommitStats.

@skuruppu
Copy link
Contributor

skuruppu commented Jan 27, 2021

@skuruppu The current build failure seems to be caused by a test project that does not (yet) support CommitStats. Running the integration tests against the Go cloud test project does not return any CommitStats, running the same tests against another dev project does return CommitStats.

Oh interesting. I think it's because it's not fully released in the backend. I'll give you a go ahead to rerun the tests when the release is complete.

Thanks for rebasing the PR :)

@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

Hmmm still failing the test but I don't understand why.

--- FAIL: TestIntegration_ReadWriteTransaction_StatementBasedWithOptions (0.13s)
    integration_test.go:1263: Missing commit stats in commit response
--- FAIL: TestIntegration_ReadWriteTransactionWithOptions (0.16s)
    integration_test.go:1086: Missing commit stats in commit response

@skuruppu
Copy link
Contributor

skuruppu commented Feb 4, 2021

@olavloite I can confirm that equivalent tests pass for all other languages so would you be able to check if it's an issue with the Go implementation?

@olavloite
Copy link
Contributor Author

olavloite commented Feb 4, 2021

@olavloite I can confirm that equivalent tests pass for all other languages so would you be able to check if it's an issue with the Go implementation?

Hmmm... This is strange. If I run the above mentioned integration tests from my local environment against a normal test instance, the integration tests succeed.

@olavloite
Copy link
Contributor Author

olavloite commented Feb 4, 2021

@olavloite I can confirm that equivalent tests pass for all other languages so would you be able to check if it's an issue with the Go implementation?

Hmmm... This is strange. If I run the above mentioned integration tests from my local environment against a normal test instance, the integration tests succeed.

Never mind, the problem is that the build is failing when it is running the integration tests against the emulator, and that does not support commit stats yet.

@olavloite olavloite added the automerge Merge the pull request once unit tests and other checks pass. label Feb 4, 2021
@gcf-merge-on-green gcf-merge-on-green bot merged commit b7c3ca6 into master Feb 4, 2021
3 checks passed
@gcf-merge-on-green gcf-merge-on-green bot deleted the support-commit-stats branch Feb 4, 2021
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Feb 4, 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 Cloud 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

3 participants