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

fix(upkeep): Batch deadletter futures into FuturesUnordered #199

Merged
merged 5 commits into from
Feb 21, 2025

Conversation

evanh
Copy link
Member

@evanh evanh commented Feb 20, 2025

To avoid sequentially sending each deadletter future, put all the futures into a batch using FuturesUnordered to have the entire batch run concurrently.

To avoid sequentially sending each deadletter future, put all the futures into a batch using
FuturesUnordered to have the entire batch run concurrently.
@evanh evanh requested a review from a team as a code owner February 20, 2025 19:23
@john-z-yang
Copy link
Member

LGTM, should we refactor this into a helper function?

@john-z-yang
Copy link
Member

Also, I noticed that this is using join_all instead of FuturesUnordered like the other PR, is this intentional?

@evanh
Copy link
Member Author

evanh commented Feb 20, 2025

@john-z-yang I changed this to use a helper function, however:

The two loops use different types: retries uses InflightActivation and deadletter uses TaskActivation. I tried to write a generic function to handle both, using closures passed in as arguments. However closures + async move do not work well together, and the only thread I could find about it didn't end up with an answer.

So, instead I changed the retry functionality to deal with TaskActivations and then I could write one function to handle both cases.

Let me know what you think. If this seems too complicated I'll revert it.

@john-z-yang
Copy link
Member

john-z-yang commented Feb 20, 2025

@john-z-yang I changed this to use a helper function, however:

The two loops use different types: retries uses InflightActivation and deadletter uses TaskActivation. I tried to write a generic function to handle both, using closures passed in as arguments. However closures + async move do not work well together, and the only thread I could find about it didn't end up with an answer.

So, instead I changed the retry functionality to deal with TaskActivations and then I could write one function to handle both cases.

Let me know what you think. If this seems too complicated I'll revert it.

@evanh Ah I see, I didn't realize that. I think the earlier version seems simpler then. However, either looks fine to me so I'll let you decide!

@evanh evanh merged commit d2b819d into main Feb 21, 2025
11 checks passed
@evanh evanh deleted the evanh/fix/batch-deadletter branch February 21, 2025 14:05
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.

3 participants