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 custom options to ApiCallContext #1435

Merged
merged 12 commits into from Aug 16, 2021

Conversation

mutianf
Copy link
Contributor

@mutianf mutianf commented Jul 26, 2021

Adding a withOption method to ApiCallContext so additional contexts can be passed in the callable chains. The idea is similar to https://github.com/grpc/grpc-java/blob/master/api/src/main/java/io/grpc/CallOptions.java#L318. The custom options are encapsulated in ApiCallCustomOptions.

An example usage is to pass batch throttled time to a tracer #1413 (comment).

@mutianf mutianf requested review from as code owners Jul 26, 2021
@google-cla google-cla bot added the cla: yes label Jul 26, 2021
@mutianf
Copy link
Contributor Author

@mutianf mutianf commented Jul 26, 2021

Hi @vam-google , creating this pr so we don't need to use instanceof as discussed in #1413 (comment). If this looks good, I'll make changes to #1413 based on this pr. Thanks!

@mutianf mutianf changed the title feat: add extra contexts to ApiCallContext feat: add custom contexts to ApiCallContext Jul 28, 2021
@mutianf mutianf force-pushed the call_context branch 2 times, most recently from 9939551 to eae5b9e Compare Jul 28, 2021
@mutianf mutianf changed the title feat: add custom contexts to ApiCallContext feat: add custom options to ApiCallContext Aug 4, 2021
Copy link
Collaborator

@igorbernstein2 igorbernstein2 left a comment

lgtm

Copy link
Contributor

@vam-google vam-google left a comment

LGTM, if we really need the Key<T> abstraction here (please clarify why we can't just use String or Object there).

@@ -491,6 +508,29 @@ public GrpcCallContext withTracer(@Nonnull ApiTracer tracer) {
return withCallOptions(callOptions.withOption(TRACER_KEY, tracer));
}

/** {@inheritDoc} */
@Override
public <T> GrpcCallContext withOption(Key<T> key, T value) {
Copy link
Contributor

@vam-google vam-google Aug 12, 2021

Choose a reason for hiding this comment

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

@BetaApi or is it not worth it?

Copy link
Contributor Author

@mutianf mutianf Aug 13, 2021

Choose a reason for hiding this comment

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

@BetaApi is added to ApiCallContext, do we still need it here? This class is also annotated with @BetaApi.

<T> T getOption(Key<T> key);

/** Key for api call context options key-value pair. */
final class Key<T> {
Copy link
Contributor

@vam-google vam-google Aug 12, 2021

Choose a reason for hiding this comment

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

Why do we need this? What is the use of type parameter T here? It does not seem it is being used in class itself (the name field is String explicitly). Can we just have "String" directly as key?

Copy link
Contributor Author

@mutianf mutianf Aug 13, 2021

Choose a reason for hiding this comment

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

The type is to make sure when withOption(Key<T> key, T value) is called, the correct value type is passed in to avoid casting problems.

if (!options.containsKey(key)) {
builder.putAll(options).put(key, value);
} else {
for (Key oldKey : options.keySet()) {
Copy link
Contributor

@vam-google vam-google Aug 12, 2021

Choose a reason for hiding this comment

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

Really feels like there must be a more elegant way of doing it, but Ok =)

Copy link
Contributor Author

@mutianf mutianf Aug 13, 2021

Choose a reason for hiding this comment

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

Updated the code a bit, does it look better now?

Copy link
Contributor

@vam-google vam-google left a comment

LGTM

@vam-google vam-google merged commit 0fe20f3 into googleapis:master Aug 16, 2021
6 checks passed
@mutianf mutianf deleted the call_context branch Aug 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants