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

fix: Add logic to ticker loop to ensure subscription always exists with an open connection #861

Merged
merged 1 commit into from
Sep 4, 2020

Conversation

terev
Copy link
Contributor

@terev terev commented Sep 4, 2020

Summary

Fixes #860

This pr ensures that a subscription exists as long as there's an active connection to the eventbus.

Manual test plan

  • This was tested locally by installing argo-events and testing my dev sensor image.
  • I started an instance of the webhook eventbus/sensor. Then I verified that events flowed to the sensor.
  • I then deleted all of the nats eventbus pods to force close the connection kubectl delete po -n argo-events -l 'eventbus-name=default'
  • Then i verified that the subscription was eventually re-established and events flow correctly
  • I repeated the deletion steps to validate that it works multiple times

I couldn't find any tests that touch this area but i could've missed them?

@terev terev changed the title Add logic to ticker loop to ensure subscription always exists with an open connection fix: Add logic to ticker loop to ensure subscription always exists with an open connection Sep 4, 2020
Copy link
Member

@whynowy whynowy left a comment

Choose a reason for hiding this comment

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

This looks great! I tested by myself as well by randomly error out the subscription. Thanks for fixing it!

@@ -39,6 +40,15 @@ import (
sensortriggers "github.com/argoproj/argo-events/sensors/triggers"
)

func subscribeOnce(subLock *uint32, subscribe func()) {
// acquire subLock if not already held
if !atomic.CompareAndSwapUint32(subLock, 0, 1) {
Copy link
Member

Choose a reason for hiding this comment

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

Learnt something, thanks!

@whynowy whynowy merged commit f2411be into argoproj:master Sep 4, 2020
whynowy pushed a commit that referenced this pull request Sep 4, 2020
juliev0 pushed a commit to juliev0/argo-events that referenced this pull request Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

If subscribe to event bus fails the sensor ends up in a state without a subscription
2 participants