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

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 a team as code owners July 26, 2021 18:56
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 26, 2021
@mutianf
Copy link
Contributor Author

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 July 28, 2021 15:42
@mutianf mutianf changed the title feat: add custom contexts to ApiCallContext feat: add custom options to ApiCallContext Aug 4, 2021
Copy link
Contributor

@igorbernstein2 igorbernstein2 left a comment

Choose a reason for hiding this comment

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

lgtm

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, 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

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

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

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

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

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

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

Choose a reason for hiding this comment

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

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants