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

chore: Require breadth >= 1 in FlattenMerge #1699

Merged
merged 1 commit into from
Jan 10, 2025
Merged

Conversation

He-Pin
Copy link
Member

@He-Pin He-Pin commented Jan 10, 2025

Motivation:
Check breadth >= 1 in FlattenMerge

Modification:
Add a requirement for that.

@He-Pin He-Pin added the t:stream Pekko Streams label Jan 10, 2025
@He-Pin He-Pin added this to the 1.2.0 milestone Jan 10, 2025
@He-Pin He-Pin merged commit 18e98d4 into apache:main Jan 10, 2025
9 checks passed
@He-Pin He-Pin deleted the breadth branch January 10, 2025 11:16
@pjfanning
Copy link
Contributor

  • is it really possible to end up in a position where the requirement fails?
  • if so, can we add a test?
  • also, what would users of older versions see in this case? an exception, a nonsensical result?

@He-Pin
Copy link
Member Author

He-Pin commented Jan 10, 2025

@pjfanning This is extract of a pending PR, I can't keep 100% code coverage at $Work too.

@raboof
Copy link
Member

raboof commented Jan 10, 2025

I think checks like this are helpful because they make it easier to read the implementation of FlattenMerge and be confident you don't need to consider the case where breadth < 1

  • is it really possible to end up in a position where the requirement fails?

That's indeed not super obvious, but I suspect it can when the user chooses a <1 parallelism somewhere.

* if so, can we add a test?

I guess so, though I'm not sure it's worth it to test error checking so explicitly

* also, what would users of older versions see in this case? an exception, a nonsensical result?

I suspect they'd see a flow which stalls (fails to pull) after accepting its first element.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t:stream Pekko Streams
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants