Skip to content

grpc: detemplatize Grpc::AsyncClient.#2376

Merged
htuch merged 3 commits intoenvoyproxy:masterfrom
htuch:gprc-templates
Jan 16, 2018
Merged

grpc: detemplatize Grpc::AsyncClient.#2376
htuch merged 3 commits intoenvoyproxy:masterfrom
htuch:gprc-templates

Conversation

@htuch
Copy link
Copy Markdown
Member

@htuch htuch commented Jan 16, 2018

In order to support a singleton gRPC manager (managing TLS state,
connection pools for the Google gRPC client, etc.), this PR removes the
use of templating in Grpc::AsyncClient and its related interfaces.

We initially used templates as they were an easy way to propagate type
information. However, we can avoid using them by taking advantage of the
Protobuf::Message superclass interface. There is some convenience in
using templating for the request/stream callbacks, adapter template
classes are provided to make this possible.

Risk Level: Low
Testing: No new tests, this is purely refactoring.

Signed-off-by: Harvey Tuch htuch@google.com

In order to support a singleton gRPC manager (managing TLS state,
connection pools for the Google gRPC client, etc.), this PR removes the
use of templating in Grpc::AsyncClient and its related interfaces.

We initially used templates as they were an easy way to propagate type
information. However, we can avoid using them by taking advantage of the
Protobuf::Message superclass interface. There is some convenience in
using templating for the request/stream callbacks, adapter template
classes are provided to make this possible.

Signed-off-by: Harvey Tuch <htuch@google.com>
};

// Templatized variant of AsyncRequestCallbacks.
template <class ResponseType> class TypedAsyncRequestCallbacks : public AsyncRequestCallbacks {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@mattklein123 this pattern might make sense also (in a separate PR) for the filter instantiation interface. These templatized adapters allow the boiler plate around having to specify a createEmptyProto() method and then later dynamic_cast to be handled by the compiler.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That sounds great. Let's do it!

htuch added 2 commits January 16, 2018 10:02
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

This is awesome, thanks. Love the direction. Way easier to read and use.

};

// Templatized variant of AsyncRequestCallbacks.
template <class ResponseType> class TypedAsyncRequestCallbacks : public AsyncRequestCallbacks {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That sounds great. Let's do it!

@htuch htuch merged commit 6d62998 into envoyproxy:master Jan 16, 2018
@htuch htuch deleted the gprc-templates branch January 16, 2018 18:29
istio-merge-robot pushed a commit to istio/proxy that referenced this pull request Jan 23, 2018
Automatic merge from submit-queue.

Update envoy to 01ee725

**What this PR does / why we need it**:
Update envoy to envoyproxy/envoy@01ee725

**Special notes for your reviewer**:
Related Envoy changes:
envoyproxy/envoy#2368
envoyproxy/envoy#2376


**Release note**:

```release-note
None
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants