Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

feat: Add a builder to handle the common logic of extracting routing header values from request #1598

Merged
merged 7 commits into from Jan 27, 2022

Conversation

blakeli0
Copy link
Contributor

@blakeli0 blakeli0 commented Jan 10, 2022

In order to support explicit dynamic routing headers: googleapis/sdk-platform-java#869, we have to match-and-extract field values from requests during runtime for each configured routing rule parameter, this PR is to add a request params builder to handle the common logics.

@blakeli0 blakeli0 requested review from a team as code owners January 10, 2022 20:40
@blakeli0 blakeli0 changed the title Add a helper to handle the common logic of extracting routing header … feat : Add a helper to handle the common logic of extracting routing header … Jan 10, 2022
@blakeli0 blakeli0 changed the title feat : Add a helper to handle the common logic of extracting routing header … feat: Add a helper to handle the common logic of extracting routing header … Jan 10, 2022
Copy link
Contributor

@chanseokoh chanseokoh left a comment

Choose a reason for hiding this comment

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

LGTM, pending minor comments. And do you plan to expand the tests?

Copy link
Member

@meltsufin meltsufin left a comment

Choose a reason for hiding this comment

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

Please try to include a javadoc comment for each public class and method. We should probably enforce it via the checkstyle.

Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

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

All but one comments are minor. Using guava on public surface is forbidden, please change that.

- Move it up to rpc package
- Add java docs.
- Rename tests.
@meltsufin meltsufin changed the title feat: Add a helper to handle the common logic of extracting routing header … feat: Add a helper to handle the common logic of extracting routing header values from request Jan 13, 2022
Copy link
Member

@meltsufin meltsufin left a comment

Choose a reason for hiding this comment

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

LGTM, pending resolution of comments.

@blakeli0 blakeli0 changed the title feat: Add a helper to handle the common logic of extracting routing header values from request feat: Add a builder to handle the common logic of extracting routing header values from request Jan 26, 2022
Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

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

LGTM, just one comment.

@blakeli0 blakeli0 merged commit 2836baa into main Jan 27, 2022
@blakeli0 blakeli0 deleted the explicit-dynamic-routing-header branch January 27, 2022 19:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants