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.

Sign up for GitHub

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

Jump to bottom

Fix event multiplexer pattern for NATS #12006

Merged
merged 4 commits into from Sep 7, 2021
Merged

Fix event multiplexer pattern for NATS #12006

merged 4 commits into from Sep 7, 2021

Conversation

ghost
Copy link

@ghost ghost commented Sep 1, 2021

Description

Changes proposed in this pull request:

  • fix the event multiplexer pattern for NATS backend
  • add tests to verify it

Related issue(s)

See also: #11719 #11989

@ghost ghost added the area/eventing Issues or PRs related to eventing label Sep 1, 2021
@ghost ghost self-assigned this Sep 1, 2021
@kyma-bot kyma-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 1, 2021
This was linked to issues Sep 1, 2021
Event delivery for NATS is broken with multiple subscriptions #11989
Closed
The "multiplexer" eventing pattern doesn't work with the NATS backend #11719
Closed
@ghost
Copy link
Author

ghost commented Sep 1, 2021

/retest pre-main-kyma-gardener-gcp-eventing

@kyma-bot
Copy link
Contributor

kyma-bot commented Sep 1, 2021 •
edited by ghost

@radufa: The /retest command does not accept any targets.

@ghost
Copy link
Author

ghost commented Sep 1, 2021

/test pre-main-kyma-gardener-gcp-eventing

@mfaizanse
Copy link
Member

Hi,

  • I have tested the PR on the cluster and the multiplexing is working. I used 3 functions and 3 subscriptions.
  • I also reproduced the bug on kyma:main, and multiplexing is not working on main.
  • Also, the unit test is failing without the fix, so thats good. And with new changes the unit test is passing.

k15r
k15r reviewed Sep 3, 2021
View reviewed changes
components/eventing-controller/pkg/handlers/nats_test.go Outdated
// Create 3 subscriptions having the same sink and the same event type
var subs [3]*eventingv1alpha1.Subscription
for i:=0; i< len (subs); i++ {
subs[i] = eventingtesting.NewSubscription("sub" + strconv.Itoa(i), "foo", eventingtesting.WithNotCleanEventTypeFilter)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: fmt is already imported, so

fmt.Sprintf("sub-%d", i)
Suggested change
subs[i] = eventingtesting.NewSubscription("sub" + strconv.Itoa(i), "foo", eventingtesting.WithNotCleanEventTypeFilter)
subs[i] = eventingtesting.NewSubscription(fmt.Sprintf("sub-%d", i), "foo", eventingtesting.WithNotCleanEventTypeFilter)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

components/eventing-controller/pkg/handlers/nats.go Show resolved Hide resolved
k15r
k15r previously approved these changes Sep 3, 2021
View reviewed changes
@kyma-bot kyma-bot added the lgtm Looks good to me! label Sep 3, 2021
@netlify
Copy link

netlify bot commented Sep 3, 2021 •
edited

✔️ 🥰 Documentation preview ready! 🥰

🔨 Explore the source changes: fcb4c0d

🔍 Inspect the deploy log: https://app.netlify.com/sites/kyma-project-docs-preview/deploys/613707f848dec60008df1c31

😎 Browse the preview: https://deploy-preview-12006--kyma-project-docs-preview.netlify.app

@ghost
Copy link
Author

ghost commented Sep 3, 2021

/test pre-main-kyma-integration-k3d-compass-dev

@ghost
Copy link
Author

ghost commented Sep 3, 2021

/retest

7 similar comments
@ghost
Copy link
Author

ghost commented Sep 3, 2021

/retest

@ghost
Copy link
Author

ghost commented Sep 3, 2021

/retest

@ghost
Copy link
Author

ghost commented Sep 5, 2021

/retest

@ghost
Copy link
Author

ghost commented Sep 6, 2021

/retest

@ghost
Copy link
Author

ghost commented Sep 6, 2021

/retest

@ghost
Copy link
Author

ghost commented Sep 6, 2021

/retest

@ghost
Copy link
Author

ghost commented Sep 6, 2021

/retest

@ghost
Copy link
Author

ghost commented Sep 6, 2021

/retest

@kyma-bot kyma-bot removed the lgtm Looks good to me! label Sep 7, 2021
@ghost
Copy link
Author

ghost commented Sep 7, 2021

/test pre-main-kyma-integration-k3d-compass-dev

@ghost
Copy link
Author

ghost commented Sep 7, 2021

/test pre-main-kyma-gke-upgrade

@kyma-bot kyma-bot added the lgtm Looks good to me! label Sep 7, 2021
@kyma-bot kyma-bot merged commit fb1ca34 into kyma-project:main Sep 7, 2021
@kyma-bot
Copy link
Contributor

kyma-bot commented Sep 7, 2021

@radufa: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Rerun command
pre-main-kyma-integration-k3d-compass-dev fcb4c0d link /test pre-main-kyma-integration-k3d-compass-dev
pre-main-kyma-gardener-azure-integration-cosigned fcb4c0d link /test pre-main-kyma-gardener-azure-integration-cosigned

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lilitgh lilitgh Awaiting requested review from lilitgh

@marcobebway marcobebway Awaiting requested review from marcobebway

@nachtmaar nachtmaar Awaiting requested review from nachtmaar

@pxsalehi pxsalehi Awaiting requested review from pxsalehi

@sayanh sayanh Awaiting requested review from sayanh

@themue themue Awaiting requested review from themue

Assignees

@k15r k15r

Labels
area/eventing Issues or PRs related to eventing lgtm Looks good to me! size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
3 participants
@kyma-bot @mfaizanse @k15r