Skip to content
This repository was archived by the owner on Dec 13, 2023. It is now read-only.

Handling messages loss case on exceptions#3585

Merged
v1r3n merged 7 commits intoNetflix:mainfrom
AvitalOfstein:feature/nackMessagesCantBeHandled
Apr 19, 2023
Merged

Handling messages loss case on exceptions#3585
v1r3n merged 7 commits intoNetflix:mainfrom
AvitalOfstein:feature/nackMessagesCantBeHandled

Conversation

@AvitalOfstein
Copy link

Pull Request type

  • [ X ] Bugfix

Changes in this PR

When message has no transient failures, only logically unhandled exception - message was being Acked. In our case - message that it's handler metadata wasn't fetched successfully (for example, Connection with Redis was down).
In this case we would expect it to-
1.Be retried (transient failure) - Handled with adding transient exception catching in RedisEventHandlerDao
2. In general, we would like only messages that passed successfully, without any type of exception - will be Acked. Messages that we want by config to be republished on failure OR have exceptions that are transient - to be published back to queue. Only other failing messages - to be Nacked.

When message has no transient failures, only logically unhandled exception - message was being Acked. In our case - message that it's handler metadata wasn't fetched successfully (Connection with Redis was down).
In this case we would expect it to-
 1.Be retried (transient failure) - Handled with adding transient exception catching in RedisEventHandlerDao
 2. In general, only messages that passed successfully, without any type of exception - will be Acked. Messages that we want by config to be republished on failure OR have exceptions that are transient - to be published back to queue. Only other failing messages - to be Nacked.
@AvitalOfstein AvitalOfstein changed the title Handling messages loss case on transient exceptions Handling messages loss case on exceptions Apr 19, 2023
@v1r3n
Copy link
Contributor

v1r3n commented Apr 19, 2023

@AvitalOfstein can you run the spotless and update the PR?

Avital Ofstein added 4 commits April 19, 2023 23:10
@v1r3n v1r3n merged commit 3981a15 into Netflix:main Apr 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants