search

knative / eventing

Event-driven application platform for Kubernetes
https://knative.dev/docs/eventing
Apache License 2.0
1.42k stars 595 forks source link

Proposal to improve eventing/channel/MessageDispatcher #7456

Closed astelmashenko closed 7 months ago

astelmashenko commented 11 months ago

Problem There is knative.dev/eventing golang library, it has channel package. There is MessageDispatcher in channel package. Simplified diagram how it is working: image

The message dispatcher is used by eventing brokers, like eventing-natss, eventing-kafka (most likely), etc. The problem is in the MessageDispatcher, what if dispatcher crashes? The message should be re-delivered by an underlying message queue/stream, for that message re-delivery should be configured on underlying stream consumer, e.g. in Nats/JetStream case there is MaxDeliver property for a consumer. The same time there are retries configured on trigger/subscription level. It is possible that there will be more re-deliveries then configured on trigger level, image message was retried 4 of 5 times and then dispatcher crashes, after restart the message is redelivered by Nats and it is possible it will be retires another 5 times. Another thing is why should a MessageDispatcher retry message in-memory if there is a feature on underlying stream consumer like MaxDeliver on JetStream? If I'd like to utilize JetStream's retry feature, then I need to totally re-implement MessageDispatcher because I do not want to lose DLQ and replyUrl functionality.

Persona: Contributor/Corporate (employed) maintainer

Exit Criteria MessageDispatcher has pluggable version of DispatchMessageWithRetries so that it is possible to identify if there are no retries on underlying stream consumer side.

Additional context (optional) Related PR https://github.com/knative-extensions/eventing-natss/pull/426

pierDipi commented 11 months ago

MessageDispatcher has pluggable version of DispatchMessageWithRetries so that it is possible to identify if there are no retries on underlying stream consumer side.

On main MessageDispatcher has been renamed to Dispatcher with an improved API for new use cases (like #6806).

Given that can we describe how the new Dispatcher API would look like to support your use case?

astelmashenko commented 11 months ago

I tried to think about about function's signature, but it is not only signature. The Send function it self should be changed. I'd say it is not necessary to try to make it more generic or accept some tricky interfaces. Instead it'd be better to make it modular, the straght forwad is try to split Send method to something like, SendToTarget, SendToDQL, etc. At least make executeRequest public. I'll try to think more on that direction.

github-actions[bot] commented 8 months ago

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.