cloudevents / sdk-go

Go SDK for CloudEvents
https://cloudevents.github.io/sdk-go/
Apache License 2.0
837 stars 223 forks source link

gRPC protocol implementation #980

Open morvencao opened 1 year ago

morvencao commented 1 year ago

The cloudevents sdk-go currently offers support for various protocol implementations, except for gRPC. It is essential to add gRPC protocol support since it is widely used.

While the cloudevent proto is presented in the repository, it lacks the RPC service definition necessary for implementing the gRPC protocol in cloud event sdk-go.

To enable gRPC functionality, defining the RPC service and method is imperative. This ensures cloudevents sdk-go to send and receive through the generated golang gRPC client.

Can we incorporate the RPC service and method into the cloudevent proto in the following manner:

image

Next, utilize protoc to generate the RPC client and server code, which can be employed to implement the gRPC protocol as follows:

# cat protocol/grpc/protocol.go
...
// protocol for grpc
// define protocol for grpc

type Protocol struct {
    client          pb.CloudEventServiceClient
    publishOption   *PublishOption
    subscribeOption *SubscribeOption
    // receiver
    incoming chan *pb.CloudEvent
    // inOpen
    openerMutex sync.Mutex

    closeChan chan struct{}
}
...

/cc @embano1 @yanmxa

morvencao commented 1 year ago

kindly ping @duglin

yanmxa commented 1 year ago

Reference: https://github.com/cloudevents/sdk-go/issues/761

embano1 commented 1 year ago

@yanmxa sorry for my delayed response on your PR, been super busy lately which impacts my availability for this SDK :/

I went through your proposal but I can't come up with a good reason to include a specific gRPC implementation as a new protocol in this repo. My personal experience with gRPC is that its implementations are highly dependent (opinionated) on the actual use case since it's an RPC-style protocol. Your service definition proposal assumes a specific setup e.g., topics, which might not hold for the majority of users. I wonder what the adoption of this protocol would be beyond an example how to build gRPC services using CloudEvents. In addition, we haven't set up the required CI pipelines to continuously keep up with the gRPC toolkit and incorporating those into our code base (PRs e.g., from protoc). This would definitely be needed as part of the PR.

I haven't seen other issues asking for a protocol binding for gRPC. proto definition related issues seemed to be ok with just having the CloudEvents proto definition to build a custom gRPC implementation, which (to emphasise) are highly opinionated and might meet our protocol implementation if we'd offer one.

Sorry if you'd invested time in your PR since I was late to respond to this issue. Usually we reason about those things in issues before filing PRs to avoid ambiguity and wasted work.

cc/ @duglin for his thoughts

morvencao commented 1 year ago

Thank you, @embano1, for your thoughtful response.

I appreciate your concerns about the inclusion of a gRPC protocol binding in the CloudEvents repo. I'd like to emphasize that our motivation for this proposal is rooted in the superior performance of gRPC compared to HTTP in various aspects.

Our objective is to build a robust transport layer that harnesses the efficiency of gRPC, particularly in implementing watch semantics. This aligns with the inherent strengths of gRPC and the benefits it offers in certain use cases. Additionally, we aim to seamlessly integrate CloudEvents into this architecture to provide a standardized messaging format.

While I understand the challenges posed by the opinionated nature of service definitions, our intention is to create a solution that caters to the majority of users. In practical scenarios, users often seek the convenience of utilizing the CloudEvents client for sending and receiving messages, and we aim to provide a service definition that accommodates these common use cases.

I acknowledge your considerations about the need for broader applicability and the potential maintenance challenges associated with incorporating gRPC into the CI pipelines. However, I firmly believe that a gRPC protocol binding will offer significant value to users seeking enhanced performance and functionality. I'm hopeful that we can further discuss and address these concerns to pave the way for the inclusion of gRPC protocol binding in CloudEvents.

Thank you again for your time and consideration.

embano1 commented 1 year ago

Thank you, @embano1, for your thoughtful response.

You're more than welcome.

Our objective is to build a robust transport layer that harnesses the efficiency of gRPC, particularly in implementing watch semantics. This aligns with the inherent strengths of gRPC and the benefits it offers in certain use cases. Additionally, we aim to seamlessly integrate CloudEvents into this architecture to provide a standardized messaging format.

May I ask who "our" and "we" represents here? To me the proposal still seems geared towards a very specific (and opinionated, which is totally fair) use case. As I wrote earlier, I haven't seen any requests around adding a gRPC protocol binding here so I'm interested in understanding whether your proposal is generic enough to be used by a large user base and how much maintenance overhead it's going to add to the SDK maintainers. Furthermore, it might constrain you from particular optimizations/changes your use case might require over time when other users start to take a dependency on this binding.

While I understand the challenges posed by the opinionated nature of service definitions, our intention is to create a solution that caters to the majority of users. In practical scenarios, users often seek the convenience of utilizing the CloudEvents client for sending and receiving messages, and we aim to provide a service definition that accommodates these common use cases.

I still wonder whether this is a real problem (no issues backing this up at the moment) or if it's too specific, thus users using our proto definitions to build their bespoke RPCs.

I acknowledge your considerations about the need for broader applicability and the potential maintenance challenges associated with incorporating gRPC into the CI pipelines. However, I firmly believe that a gRPC protocol binding will offer significant value to users seeking enhanced performance and functionality. I'm hopeful that we can further discuss and address these concerns to pave the way for the inclusion of gRPC protocol binding in CloudEvents.

Again, can you back up performance and functionality concerns with concrete user anecdotes or data points?

Thank you again for your time and consideration.

My pleasure, please keep the discussion going.

morvencao commented 1 year ago

@embano1 Apologies for the confusion, in this context, 'we' refers to a team at Red Hat. Our objective is to develop a gRPC server with functionality akin to kube-apiserver. This server should enable the application of resources and provide a mechanism for monitoring the status changes of these resources. The server mainly need to support two key operations: publishing a resource and subscribing to resource status changes. To ensure seamless integration with other systems, we plan to adopt cloudevents as a standardized messaging format. This establishes a straightforward use case: applying resources to the server and monitoring their status changes against it. Per my understanding, the majority of cloudevents clients typically focus on two primary operations: publishing events and subscribing to events. I believe the proposed gRPC service definition adequately addresses the needs of most users, make sense?

duglin commented 1 year ago

@morvencao If I'm reading this properly, it feels a little bit odd to have the SDK choose one particular RPC signature over all others as the one to promote for all of gRPC users - even if it might be a very popular one for event streaming. It would be like the golang http package trying to promote one particular HTTP API rather than being a generic framework that has no opinion on how your HTTP APIs looks.

However, having said that, I wonder if it would be better to convert your gRPC definition into a sample that people could use 'as-is' in their project, or modify to fit their needs if their RPC signature differs. I think part of your goal is to make it easier for future gRPC/CE users by providing a jump-start gRPC/proto definition so they don't need to reinvent the wheel - and a sample might help with that.

I'm not 100% sure where some of your files (like protocol/grpc/message.go) would go though... I'm not that familiar with gRPC/proto, so it's not clear to me if that would be part of the sample or if it's generic enough to be useful for all proto/CE users. It feels a bit like the proto binding should already cover most of that, no?

Or am I not following what you're proposing?

morvencao commented 1 year ago

I completely understand.

The initial concept drew inspiration from Pub/Sub, evident in the Pub/Sub proto definition, where the primary interfaces involve publishing messages and subscribing to messages.

I've been grappling with finding an optimal solution that leverages gRPC performance while adhering to the cloudevents standard. Integrating it into a sample would be excellent, as you mentioned. However, I'm uncertain about the code tree organization. It seems unprecedented, with no other protocol binding attempting this.

The central question revolves around the placement of the RPC definition. Since it refers to the CloudEvent proto message, is it acceptable to include it in the same proto file?