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

[Host] Consumer error response per transport (#356) #359

Merged
merged 1 commit into from
Jan 3, 2025

Conversation

EtherZa
Copy link
Contributor

@EtherZa EtherZa commented Jan 2, 2025

@zarusz I was thinking about how to cleanly implement the message header feature (#359) which had me re-evaluate how the Abandon (#356) was implemented.

Abandon is only possible when using ASB and RabbitMQ, but other (future) transports may have their own requirements. As such, having an Abandon response in the ProcessResult enum was probably a little heavy handed. I have refactored the code to rather make use of specialized types to differentiate between available responses (and hopefully make the action more clear). In this way, each provider can now support the base responses (Failure, Success, SuccessWithResponse and Retry) while also adding speciailized support fot the anything available to the transport eg. DeadLetter (previously Abandon) or Failure(IDictionary<string, object> headers) for ASB. (Failure(...) has not been added in this PR, but is an example that should now be trivial to add).

RabbitMq looses Abandon but gains Requeue with the configuration option no longer being required.

Thoughts?

@EtherZa
Copy link
Contributor Author

EtherZa commented Jan 2, 2025

@zarusz Tests Amazon SQS are failing.

Queue SMB_638714009108838766_BasicQueue has unknown URL at this point. Ensure the queue exists in Amazon SQS.

@zarusz
Copy link
Owner

zarusz commented Jan 2, 2025

The flakiness observed in the build (e.g., Queue SMB_638714009108838766_BasicQueue has unknown URL at this point) might simply require retrying the build or force-pushing to re-trigger it. The int test seems flaky.

Yes, each transport does it in its own way, so part of me thought that adding extension methods on IConsumerContext to perform transport specific logic would be the most versatile way (like we have .Ack()/.Nack() for Rabbit).

As for the ASB headers, it would be good to also allow to set the headers in the regular IConsumer, as the message processing is being performed. The benefit here is that in case of a transient timeout on SQL it would return and one could just have the state restored with next retry (in headers). In one of my projects we have consumers that adapt some downstream system and create a bunch of entities in external systems.

I am writing this comment without reviewing this PR yet, will try to do so soon.

@zarusz
Copy link
Owner

zarusz commented Jan 2, 2025

@EtherZa I've reviewed the PR, and this second iteration looks much cleaner. It effectively allows us to define transport-specific response actions. I'm happy to merge it once the build successfully completes.

@EtherZa EtherZa force-pushed the feature/356-dead-letter-pass-2 branch from cddef3d to 0e18604 Compare January 3, 2025 02:31
@EtherZa EtherZa force-pushed the feature/356-dead-letter-pass-2 branch from 0e18604 to 0ffebc9 Compare January 3, 2025 02:48
@EtherZa
Copy link
Contributor Author

EtherZa commented Jan 3, 2025

@EtherZa I've reviewed the PR, and this second iteration looks much cleaner. It effectively allows us to define transport-specific response actions. I'm happy to merge it once the build successfully completes.

Success!

@zarusz zarusz merged commit d04154f into zarusz:release/v3 Jan 3, 2025
3 checks passed
@zarusz zarusz mentioned this pull request Jan 3, 2025
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.

2 participants