eventing icon indicating copy to clipboard operation
eventing copied to clipboard

Proposal to improve eventing/channel/MessageDispatcher

Open astelmashenko opened this issue 2 years ago • 3 comments

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

astelmashenko avatar Nov 16 '23 14:11 astelmashenko

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?

pierDipi avatar Nov 24 '23 06:11 pierDipi

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.

astelmashenko avatar Dec 06 '23 15:12 astelmashenko

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.

github-actions[bot] avatar Mar 06 '24 01:03 github-actions[bot]