Kafka and Natss Channels Redelivery

[pierDipi]

Fixes #933
Fixes #1105
Fixes #753
Fixes #596

Proposed Changes

• Kafka consumer exponential backoff retries
• E2E test for Kafka Channel redelivery
• Fix Natss Channel redelivery
• E2E test for Natss Channel redelivery

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pierDipi
To complete the pull request process, please assign harwayne
You can assign the PR to them by writing /assign @harwayne in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[knative-prow-robot]

Hi @pierDipi. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[mattmoor]

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

[mattmoor]

Produced via:
prettier --write --prose-wrap=always $(find -name '*.md' | grep -v vendor | grep -v .github | grep -v docs/cmd/)

[lionelvillard]

nice!

/ok-to-test

[lionelvillard]

we want to rely on the CloudEvent SDK to do the retry. Also look at the delivery spec for additional strategies.

[slinkydeveloper]

Not here, because we use bindings apis so we don't rely on client

[slinkydeveloper]

@pierDipi thanks for starting tackling this, but I'm not sure this should be solved at kafkachannel level...
IMO this is a problem that should be solved from the dispatcher, specifically straight from the message sender in kncloudevents package

[slinkydeveloper]

And I would love to explore a library before implementing this manually

[knative-metrics-robot]

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-contrib-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
kafka/channel/pkg/dispatcher/dispatcher.go	58.9%	61.2%	2.3
kafka/common/pkg/kafka/consumer_handler.go	88.9%	88.5%	-0.4

[slinkydeveloper]

This also gives us the ability to have fine grained retries: eg if the destination accepts the message but the reply fails, there is no point to retry again on destination

[knative-prow-robot]

@pierDipi: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-knative-eventing-contrib-build-tests	05ce793	link	/test pull-knative-eventing-contrib-build-tests

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[pierDipi]

This also gives us the ability to have fine grained retries: eg if the destination accepts the message but the reply fails, there is no point to retry again on destination

Agree

I would love to explore a library before implementing this manually

We can use k8s.io/apimachinery/pkg/util/wait package for linear and exponential back-off
Note: the package has only a function called ExponentialBackoff but we can actually implement the Linear back-off.

[slinkydeveloper]

We can use k8s.io/apimachinery/pkg/util/wait package for linear and exponential back-off
Note: the package has only a function called ExponentialBackoff but we can actually implement the Linear back-off.

yeah i think it's definitely a good idea, given that we already depend on this package it's even better

[slinkydeveloper]

I think you should close this and work on dispatcher directly, in order to have the retry transparent to the channel implementation

[pierDipi]

I think e2e tests here are needed

[pierDipi]

Or should we move them to eventing and use like TestBrokerChannelFlow?

[slinkydeveloper]

Yeah maybe we should have them directly in eventing