Add custom retry message processing for DLQ #1289
Conversation
…implement custom retry logic
This reverts commit 9ae6d24.
GAlexIHU
force-pushed
the
feat/batch-ingest-events-for-balance-calculation
branch
from
2a9c906
to
15ba18c
Compare
|
|
||
| // DLQ provides a middleware that salvages unprocessable messages and published them on a separate topic. | ||
| // The main middleware chain then continues on, business as usual. | ||
| func DLQ(pub message.Publisher, topic string, msgProcessor DLQMsgProcessorFunc) (message.HandlerMiddleware, error) { |
There was a problem hiding this comment.
I don't think it's worth creating a DLQ implementation for this. I would rather have the following logic in the event handler:
- try to calculate the entitlements
- collect the successfully and unsuccessfully generated entitlements
- publish the successfully generated snapshots
- publish a EventBatchedIngest event with the remainder into the dlq topic
@sagikazarmark what do you think?
@GAlexIHU Given that I did a lot of refactoring around the handlers, I can take over this PR and wrap it up if you are fine with this.
There was a problem hiding this comment.
Sounds good to me, although I don't have a lot of context.
What you are suggesting sounds like "business logic" to me. DLQ is generally a good approach for blindly retrying without clogging up the original queue.
The above proposal sounds like a more "clever" retry approach where we only retry what needs to be retried. That sounds like business logic to me and DLQ may not be the best approach there.
There was a problem hiding this comment.
@turip I'm okay with you taking over, I've left this PR stale for the past some time no not much lost there
on the DLQ implementation topic, I think having a generic RetryableError or something of the sort will only make more sense as we go forward, but I'm fine either way for now
| spec.ComposeResourcePath(entitlement.Namespace, spec.EntityEvent), | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| // TODO: add error information too | ||
| handlingError = multierror.Append(handlingError, err) |
There was a problem hiding this comment.
Can we use errors.Join instead?
|
|
||
| // DLQ provides a middleware that salvages unprocessable messages and published them on a separate topic. | ||
| // The main middleware chain then continues on, business as usual. | ||
| func DLQ(pub message.Publisher, topic string, msgProcessor DLQMsgProcessorFunc) (message.HandlerMiddleware, error) { |
There was a problem hiding this comment.
Sounds good to me, although I don't have a lot of context.
What you are suggesting sounds like "business logic" to me. DLQ is generally a good approach for blindly retrying without clogging up the original queue.
The above proposal sounds like a more "clever" retry approach where we only retry what needs to be retried. That sounds like business logic to me and DLQ may not be the best approach there.
Overview
Let's continue the DLQ implementation discussion & changes over here (branched form #1280) to unblock the ingest event batching