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

Remove bespoke SendMessage implementation #543

Open
dan-j opened this issue Apr 14, 2024 · 2 comments
Open

Remove bespoke SendMessage implementation #543

dan-j opened this issue Apr 14, 2024 · 2 comments

Comments

@dan-j
Copy link
Contributor

dan-j commented Apr 14, 2024

Problem
Copying the kncloudevents.Dispatcher smells a bit, and I have a proposal to remove it.

We can implement our own WithRetryConfig and WithDeadLetterSink options which control the dispatcher based on current state. The one thing I have a solution for, but 50/50 if I like it, is signalling when to nak/term messages...

I have this bit of code in WIP:

func ReportWithoutRetry(result *error, attempt int, cfg *kncloudevents.RetryConfig) *kncloudevents.RetryConfig {
	if cfg == nil {
		return nil
	}

	next := *cfg

	next.CheckRetry = func(ctx context.Context, resp *http.Response, err error) (bool, error) {
		retry, err := cfg.CheckRetry(ctx, resp, err)

		if retry {
			if cfg.Backoff != nil {
				multierr.AppendInto(result, NewDelayNak(cfg.Backoff(attempt, resp)))
			} else {
				multierr.AppendInto(result, protocol.ResultNACK)
			}
		}

		return false, err
	}

	return &next
}

We take in the RetryConfig defined on the subscription, and decorate it with our own CheckRetry function which always returns false but sets a protocol.ResultNACK error. You can then use it like so:

var retryError error

defer func() {
	err = msg.Finish(multierr.Append(err, retryError))
}()

...

dispatchExecutionInfo, err := c.dispatcher.SendMessage(
	ctx,
	internal.WithoutFinish(msg),
	c.sub.Subscriber,
	internal.WithRetryReporter(&retryError, attempt, c.sub.RetryConfig),
	internal.WithDeadLetterOnLastAttempt(attempt, c.sub.RetryConfig, c.sub.DeadLetter),
	kncloudevents.WithReply(c.sub.Reply),
	kncloudevents.WithTransformers(&te),
)

The finish method of the message (see #542) then understands how to check the errors to determine whether to nack, and if so what delay.

At the moment, my dead-letter handling will just ack the message. I'm not sure if this should result in a Term or not?

Persona:

  • System Integrator

Exit Criteria
No copy/paste of kncloudevents.

Time Estimate (optional):
1 day

Copy link

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 github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 14, 2024
@astelmashenko
Copy link
Member

/remove-lifecycle stale

@knative-prow knative-prow bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants