Skip to content

Add :nack strategy#86

Merged
josevalim merged 2 commits into
elixir-broadway:mainfrom
acco:acco/implement--nack-strategy-in-broadway-sqs
May 30, 2025
Merged

Add :nack strategy#86
josevalim merged 2 commits into
elixir-broadway:mainfrom
acco:acco/implement--nack-strategy-in-broadway-sqs

Conversation

@acco

@acco acco commented May 29, 2025

Copy link
Copy Markdown
Contributor

Hey team! This PR introduces a new :nack acknowledgement mode for BroadwaySQS.

The client now supports changing message visibility in addition to deleting messages, with batch requests to SQS. {:nack, timeout} can be used for :on_success or :on_failure, validated by a new custom option type.

Producer documentation explains the behavior, and tests cover the updated logic.

Let me know what you think! Happy to change the name or interface for this option.

@acco acco changed the title Refactor nack handling and update docs Add :nack strategy to BroadwaySQS May 29, 2025
@acco acco marked this pull request as draft May 29, 2025 23:07
@acco acco force-pushed the acco/implement--nack-strategy-in-broadway-sqs branch from 582797e to d7871d2 Compare May 29, 2025 23:17
With the ack option `{:nack, timeout}`, add support for "nack"ing SQS messages (setting visibility timeout to some point in the future)
@acco acco force-pushed the acco/implement--nack-strategy-in-broadway-sqs branch from d7871d2 to dd0ea5a Compare May 29, 2025 23:25
@acco acco changed the title Add :nack strategy to BroadwaySQS Add :nack strategy May 29, 2025
@acco acco marked this pull request as ready for review May 29, 2025 23:35
Comment thread lib/broadway_sqs/ex_aws_client.ex Outdated
Comment on lines +45 to +46
collect_messages_to_nack(successful, ack_options, :on_success) ++
collect_messages_to_nack(failed, ack_options, :on_failure)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we write it in the same style as above for consistency?

Suggested change
collect_messages_to_nack(successful, ack_options, :on_success) ++
collect_messages_to_nack(failed, ack_options, :on_failure)
Enum.flat_map(successful, &nack(&1, ack_options, :on_success)) ++
Enum.flat_map(failed, &nack(&1, ack_options, :on_failure))

And then nack either returns an empty list or a single element list with message and timeout. :)

@acco acco force-pushed the acco/implement--nack-strategy-in-broadway-sqs branch from dd0ea5a to b954dd5 Compare May 30, 2025 18:29
@acco acco force-pushed the acco/implement--nack-strategy-in-broadway-sqs branch from b954dd5 to 6b3e466 Compare May 30, 2025 18:34
@acco

acco commented May 30, 2025

Copy link
Copy Markdown
Contributor Author

@josevalim Good call! Addressed in 6b3e466

Verified by starting a Broadway pipeline with on_failure: {:nack, 20} and confirming that I re-received failing SQS messages every ~20s

@josevalim josevalim closed this May 30, 2025
@josevalim josevalim reopened this May 30, 2025
@josevalim josevalim merged commit 94ccc7e into elixir-broadway:main May 30, 2025
1 of 3 checks passed
@josevalim

Copy link
Copy Markdown
Contributor

💚 💙 💜 💛 ❤️

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