-
Notifications
You must be signed in to change notification settings - Fork 107
feat: Add a builder to handle the common logic of extracting routing header values from request #1598
Conversation
…values from request.
There was a problem hiding this 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?
gax/src/main/java/com/google/api/gax/rpc/internal/RoutingHeaderHelper.java
Outdated
Show resolved
Hide resolved
gax/src/test/java/com/google/api/gax/rpc/internal/RoutingHeaderHelperTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
gax/src/main/java/com/google/api/gax/rpc/internal/RoutingHeaderHelper.java
Outdated
Show resolved
Hide resolved
gax/src/test/java/com/google/api/gax/rpc/internal/RoutingHeaderHelperTest.java
Outdated
Show resolved
Hide resolved
gax/src/test/java/com/google/api/gax/rpc/internal/RoutingHeaderHelperTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
gax/src/main/java/com/google/api/gax/rpc/internal/RoutingHeaderHelper.java
Outdated
Show resolved
Hide resolved
gax/src/main/java/com/google/api/gax/rpc/internal/RoutingHeaderHelper.java
Outdated
Show resolved
Hide resolved
gax/src/main/java/com/google/api/gax/rpc/internal/RoutingHeaderHelper.java
Outdated
Show resolved
Hide resolved
gax/src/main/java/com/google/api/gax/rpc/internal/RoutingHeaderHelper.java
Outdated
Show resolved
Hide resolved
gax/src/test/java/com/google/api/gax/rpc/internal/RoutingHeaderHelperTest.java
Outdated
Show resolved
Hide resolved
gax/src/main/java/com/google/api/gax/rpc/internal/RoutingHeaderHelper.java
Outdated
Show resolved
Hide resolved
- Move it up to rpc package - Add java docs. - Rename tests.
There was a problem hiding this 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.
gax-grpc/src/main/java/com/google/api/gax/grpc/RoutingHeaderParamsBuilder.java
Outdated
Show resolved
Hide resolved
gax-grpc/src/main/java/com/google/api/gax/grpc/RoutingHeaderParamsBuilder.java
Outdated
Show resolved
Hide resolved
gax-grpc/src/main/java/com/google/api/gax/grpc/RoutingHeaderParamsBuilder.java
Outdated
Show resolved
Hide resolved
gax-grpc/src/test/java/com/google/api/gax/grpc/RoutingHeaderParamsBuilderTest.java
Outdated
Show resolved
Hide resolved
… public create() method for instantiating the builder. Add more Javadoc.
There was a problem hiding this 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.
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.