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: Allow sending committable from Unfold #371

Merged
merged 10 commits into from
Sep 26, 2024

Conversation

mj0nez
Copy link
Contributor

@mj0nez mj0nez commented May 21, 2024

This PR aligns the Unbatch and Unfold strategies to the behavior of their counterparts Batch and Reduce.
Unbatch now just submits the values of a submitted ValuesBatch one after another to the next step. Therefore, the logic was reduced to rely fully on the passed generator function for building a Message's Value.

I tried my best fixing the typing issues but there is still one in arroyo/processing/strategies/batching.py:116 with which I had no luck.

Closes #369

@mj0nez mj0nez requested review from a team as code owners May 21, 2024 21:52
arroyo/processing/strategies/batching.py Show resolved Hide resolved
next_message = Message(Value(value, {}, message.timestamp))
for value in iterable:
next_message = Message(
value=cast(Value[TOutput], value),
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this cast should be necessary, more likely somewhere else the types weren't updated

@untitaker
Copy link
Member

@mj0nez I haven't looked at the PR in detail yet since your recent changes, do you need help with the types, or what is the status? I can make some room this week to continue it if so

@mj0nez
Copy link
Contributor Author

mj0nez commented Jul 8, 2024

@mj0nez I haven't looked at the PR in detail yet since your recent changes, do you need help with the types, or what is the status? I can make some room this week to continue it if so

Yeah... it would be great if you could take over.

@mj0nez
Copy link
Contributor Author

mj0nez commented Aug 7, 2024

@untitaker Did you get a chance to look at the PR? :)

@untitaker untitaker changed the title fix: message handling in unfold / unbatch fix: Allow sending committable from Unfold Sep 26, 2024
@untitaker untitaker merged commit 5bc16a8 into getsentry:main Sep 26, 2024
14 checks passed
@untitaker
Copy link
Member

discard my earlier comment. this is done now. thanks!

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.

Unfold removes committable from values in ValueBatch
2 participants