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

Added Redis retry support. #6

Merged
merged 14 commits into from
Dec 13, 2024
Merged

Conversation

jdgjsag67251
Copy link
Contributor

Added retry functionality to Redis to prevent an event from potentially being dropped

Copy link
Owner

@radekmie radekmie left a comment

Choose a reason for hiding this comment

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

Also, could you share how often it happens to you? We've never witnessed anything like it.

@jdgjsag67251
Copy link
Contributor Author

Also, could you share how often it happens to you? We've never witnessed anything like it.

We run this on Azure Container Apps and for some inexplicable reason that just randomly drops connections.
We've contacted Microsoft support and they are looking into it. In the mean time this change made it work for us

@rikvdlooi
Copy link

Azure:

image

Every 5 minutes it loses connection. With this fix applied we do not see any issues anymore.
This is an issue with Azure (Container Apps), however their engineers are unable to locate the issue on their end. This PR fixes it on the app side.

@radekmie
Copy link
Owner

Sorry but I couldn't resist the urge to adjust it a little 😅 I did the following:

  • In README.md:
    • Changed the links to redis docs to link the exact version we're using.
    • Removed defaults if they came from redis (I assume it's self explanatory via the docs link).
    • Sorted flags.
  • In configuration:
    • Moved ConnectionManagerConfig to Config class. (There's clone involved, but it's very cheap and allowed me centralize it there in one field. In the future I'd like to use clap or something similar.)
  • In Redis connection:
    • Inlined most of the logic back into publish. I did that only because I changed the retry loop a little and I think it's not "too big" to fit in there.

Overall, I'm really happy with the way it works! Please, do test it and let me know if it's all good now. If it will, I'll publish a new release on Friday.

Thanks again!

@radekmie radekmie added the enhancement New feature or request label Dec 11, 2024
@radekmie radekmie changed the title Redis retry support Added Redis retry support. Dec 11, 2024
@jdgjsag67251
Copy link
Contributor Author

Sorry but I couldn't resist the urge to adjust it a little 😅 I did the following:

* In `README.md`:
  
  * Changed the links to `redis` docs to link the exact version we're using.
  * Removed defaults if they came from `redis` (I assume it's self explanatory via the docs link).
  * Sorted flags.

* In configuration:
  
  * Moved `ConnectionManagerConfig` to `Config` class. (There's `clone` involved, but it's very cheap and allowed me centralize it there in one field. In the future I'd like to use `clap` or something similar.)

* In Redis connection:
  
  * Inlined most of the logic back into `publish`. I did that only because I changed the retry loop a little and I think it's not "too big" to fit in there.

Overall, I'm really happy with the way it works! Please, do test it and let me know if it's all good now. If it will, I'll publish a new release on Friday.

Thanks again!

Oh yeah no of course, it's your project after all =). Thanks for the quick response!
Seems to work well with the new changes, so I'm also very happy!

@radekmie radekmie merged commit f0593e0 into radekmie:master Dec 13, 2024
1 check passed
@jdgjsag67251 jdgjsag67251 deleted the redis-retry branch December 13, 2024 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants