-
Notifications
You must be signed in to change notification settings - Fork 1
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: Use durable consumer for GitHub webhook events #349
Conversation
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more Footnotes
|
server/application-server/src/main/java/de/tum/cit/aet/helios/nats/NatsConsumerService.java
Outdated
Show resolved
Hide resolved
server/application-server/src/main/java/de/tum/cit/aet/helios/nats/NatsConsumerService.java
Outdated
Show resolved
Hide resolved
server/application-server/src/main/java/de/tum/cit/aet/helios/nats/NatsConsumerService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for dealing with our NATS configuration. I've tested NATS as described in the description. Everything seems to work perfectly. I believe that this one will resolve our NATS heartbeat errors in prod, since it's increasing thresholds and also reinitializing the durable consumer.
Thanks for the great work and detailed description of the PR! I've added 1-2 comments. Let's merge this one soon 🚀
server/application-server/src/main/java/de/tum/cit/aet/helios/nats/NatsConsumerService.java
Outdated
Show resolved
Hide resolved
|
||
private final NatsConsumerService natsConsumerService; | ||
|
||
@Autowired |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this Autowired annotation is unnecessary because Spring automatically autowires the only constructor of a bean class, even if it's not annotated. Since NatsErrorListener has only one constructor, we can remove the annotation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't really sure about whether the Autowired
should stay or not since between the NatsErrorListener
and NatsConsumerService
we have a circular dependency and all the documentaiton and articles bout solving the issue was pointing me out to also use Autowired
with it.
Also the javadocs of @Lazy
annotation is also suggest me to use it with Autowired
I mean i think when we don't use Autowired
on the constructor and when spring was deciding on the bean initialization and injections on the startup i think if a class has only one constructor then it doesn't have to have Autowired
annotation.
I also tested without the Autowired
and it worked 🥳
But i would just propose to keep it in here if that makes sense 😄
Motivation
Currently, we are using an ephemeral consumer, which causes all webhook events to be reprocessed from scratch whenever we redeploy. Switching to a durable consumer ensures event processing continuity and may help resolve the connection issues experienced during our launch. Additionally, these changes lay the foundation for future runtime updates to consumer subjects.
Furthermore, to handle cases where NATS unexpectedly removes our durable consumer (e.g., due to inactivity or other issues), we introduced a
NatsErrorListener
that detects consumer deletion errors and triggers consumer reinitialization.Description
NATS_DURABLE_CONSUMER_NAME
environment variable.nats:2.10.25-alpine
instead ofnats:alpine
for consistency with production. This ensures that features likecreateOrUpdateConsumer
work as expected.updateSubjects
method that triggerssetupOrUpdateConsumer
with the latest subjects. This method is crucial for updating consuming subjects at runtime for another PR feat: Dynamic repo registration #351.setupOrUpdateConsumer
andupdateSubjects
synchronized
inNatsConsumerService
to ensure thread safety during runtime subject updates.NatsErrorListener
to detect critical NATS errors, such asConsumer Deleted (409)
errors.NatsErrorListener
logs the event and callsreinitializeConsumer
fromNatsConsumerService
to recreate the consumer automatically.NatsConsumerService
andNatsErrorListener
by using setter injection with@Lazy
and@Autowired
annotations to break the dependency loop while ensuring correct bean initialization.NATS_CONSUMER_INACTIVE_THRESHOLD_MINUTES
(default: 30) specifies the time (in minutes) after which an inactive consumer is removed.NATS_CONSUMER_ACK_WAIT_SECONDS
(default: 60) specifies the time (in seconds) that NATS waits for a message acknowledgment before resending the message.Durable Consumer Behavior and Runtime Updates
updateSubjects
method allows the consumer to update the subjects it listens to at runtime. This is essential when the GitHub App is installed on new repositories or removed from existing ones.Testing Instructions
Durable stream
.env
file undersrc/application-server
and setNATS_DURABLE_CONSUMER_NAME
.Reconnect (recreate) Consumer
NATS_DURABLE_CONSUMER_NAME
so repeat below steps for bothConsumer Name: (randomly generated or durable name)
Stream: github
docker run --rm -it --network=host natsio/nats-box
github
Streamnats consumer list github --server=nats://<NATS_AUTH_TOKEN>@localhost:4222
NATS_AUTH_TOKEN
defined in your project root.env
fileFind and see it exist.
nats consumer delete github <you-consumer-name> --server=nats://<NATS_AUTH_TOKEN>@localhost:4222
Consumer Deleted
then the sequesnce data is removed from NATS and when you reinitialize your consumer it starts sending events from the beginning (config).