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 x-goog-request-params to header #940

Merged
merged 1 commit into from
Dec 19, 2022

Conversation

kolea2
Copy link
Collaborator

@kolea2 kolea2 commented Dec 16, 2022

No description provided.

@kolea2 kolea2 requested review from a team as code owners December 16, 2022 17:44
@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: datastore Issues related to the googleapis/java-datastore API. labels Dec 16, 2022
Copy link
Member

@viacheslav-rostovtsev viacheslav-rostovtsev left a comment

Choose a reason for hiding this comment

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

LGTM, but check the comment

@@ -46,6 +47,8 @@ class RemoteRpc {
@VisibleForTesting static final String API_FORMAT_VERSION_HEADER = "X-Goog-Api-Format-Version";
private static final String API_FORMAT_VERSION = "2";

@VisibleForTesting static final String X_GOOG_REQUEST_PARAMS_HEADER = "x-goog-request-params";

Choose a reason for hiding this comment

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

This makes me think that we should expose this key as public in gax-java in the future, we have the same key defined here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@blakeli0 makes sense! This module doesn't depend on gax, but let me know if you'd like any similar changes here.

@blakeli0
Copy link

I know we probably only need databaseId and projectId at this moment, but in case we need more info that we cannot easily get from the request directly in the future, do we want to implement extract and match logic based on the proto? So that we don't need to change the code every time the routing annotation changes.
We implemented it for pure gRPC gapic client libraries, and this is how the autogenerated client for Bigtable look like(I know Bigtable does not really use it, but you get the idea).

@kolea2
Copy link
Collaborator Author

kolea2 commented Dec 19, 2022

I know we probably only need databaseId and projectId at this moment, but in case we need more info that we cannot easily get from the request directly in the future, do we want to implement extract and match logic based on the proto? So that we don't need to change the code every time the routing annotation changes. We implemented it for pure gRPC gapic client libraries, and this is how the autogenerated client for Bigtable look like(I know Bigtable does not really use it, but you get the idea).

@blakeli0 this is an interesting idea - I will file an issue to look into this as a refactor for the future

@kolea2 kolea2 added the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 19, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 19, 2022
@triplequark triplequark merged commit dee8cb4 into googleapis:multi-db Dec 19, 2022
kolea2 added a commit to kolea2/java-datastore that referenced this pull request May 24, 2023
gcf-merge-on-green bot pushed a commit that referenced this pull request Jun 8, 2023
This PR consists of cherry picks from the https://togithub.com/googleapis/java-datastore/tree/multi-db branch. They include:

#928 
#940 
#942 

This also enables parameterized testing for ITDatastoreTest.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the googleapis/java-datastore API. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants