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

gRPC protocol implementation #980

Open
morvencao opened this issue Nov 7, 2023 · 8 comments · May be fixed by #984
Open

gRPC protocol implementation #980

morvencao opened this issue Nov 7, 2023 · 8 comments · May be fixed by #984

Comments

@morvencao
Copy link

morvencao commented Nov 7, 2023

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 morvencao linked a pull request Nov 9, 2023 that will close this issue
@morvencao
Copy link
Author

kindly ping @duglin

@yanmxa
Copy link
Contributor

yanmxa commented Nov 12, 2023

Reference: #761

@embano1
Copy link
Member

embano1 commented Nov 15, 2023

@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
Copy link
Author

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
Copy link
Member

embano1 commented Nov 17, 2023

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
Copy link
Author

morvencao commented Nov 20, 2023

@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
Copy link
Contributor

duglin commented Nov 22, 2023

@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
Copy link
Author

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?

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 a pull request may close this issue.

4 participants