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

spanner: set google-cloud-resource-prefix metadata #332

Conversation

danielnorberg
Copy link
Contributor

Set google-cloud-resource-prefix metadata to speed up spanner data RPCs.

It seems that data RPCs still work without this, albeit with surprisingly reduced performance. I have not found anything in the spanner RPC API docs to indicate which RPCs should have this metadata set but the golang spanner client seems to be setting it for all session creation and data RPCs.

Not sure what is the preferred way to implement this. This PR adds a metadata field on crate::apiv1::spanner_client::Client to avoid having to pass it in to the public methods on the impl.

Other approaches could be to instead explicitly pass in the metadata from call sites, and/or infer it from already available information like database fields on the request types when available.

#323 (comment)

See:

@danielnorberg danielnorberg marked this pull request as ready for review December 21, 2024 16:55
@danielnorberg
Copy link
Contributor Author

Tested the latency of the BeginTransaction and ExecuteStreamingSql RPCs and they also seem to be affected by the presence of google-cloud-resource-prefix same as the session creation RPCs, so it seems prudent to include this metadata for every database-scoped RPC, same as the golang client does.

@yoshidan yoshidan added the safe to test safe to test label Dec 23, 2024
@yoshidan
Copy link
Owner

Thank you for your contribution. I will fix lint error

@yoshidan yoshidan merged commit 23f350b into yoshidan:main Dec 23, 2024
7 of 9 checks passed
@danielnorberg
Copy link
Contributor Author

Sorry about the lint error, pushed a fix here: #334

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test safe to test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants