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

BEAM-5993 Create SideInput Load test #7020

Merged
merged 2 commits into from Dec 19, 2018

Conversation

kkucharc
Copy link
Contributor

Creating Synthetic Source load test of Side Input.


Follow this checklist to help us incorporate your contribution quickly and easily:

  • [ x ] Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

It will help us expedite review of your Pull Request if you tag someone (e.g. @username) to look at it.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- --- --- --- ---
Java Build Status Build Status Build Status Build Status Build Status Build Status Build Status Build Status
Python Build Status --- Build Status
Build Status
Build Status --- --- ---

@kkucharc kkucharc force-pushed the BEAM-5993-side-input-load-test-python branch from e2f4b4a to 5a5362f Compare November 13, 2018 16:01
@pabloem
Copy link
Member

pabloem commented Nov 29, 2018

Do you need a review for this? : )

@kkucharc
Copy link
Contributor Author

@pabloem I would really appreciate it! I think there is a lot to change, but I'm wondering what's your opinion.

)
# pylint: disable=expression-not-assigned
(main_input
| "Merge" >> beam.ParDo(
Copy link
Member

Choose a reason for hiding this comment

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

By iterating through the side input in "Merge", you will consume it, and no longer need to iterate through it in "Consume" - so it may be fine if you just drop the Consume transform. Other than that, I think this must be fine : )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds good. Thanks! Consume was useful to check if join_fn correctly merges together main_input and side input. But you are right - now it's not necessary.

@pabloem
Copy link
Member

pabloem commented Dec 11, 2018

This looks fine to me. I'm glad to merge it. I believe you've run the pipeline right?

@kkucharc kkucharc force-pushed the BEAM-5993-side-input-load-test-python branch 2 times, most recently from 2ef84d0 to 9e66b0b Compare December 11, 2018 15:22
@kkucharc
Copy link
Contributor Author

@pabloem Locally - yes. But probably as you can see it started failing with PreCommit now. I'm investigating this.

@kkucharc kkucharc force-pushed the BEAM-5993-side-input-load-test-python branch from 9e66b0b to 453ed35 Compare December 17, 2018 16:26
@kkucharc kkucharc force-pushed the BEAM-5993-side-input-load-test-python branch from 453ed35 to 7344634 Compare December 18, 2018 11:58
@kkucharc
Copy link
Contributor Author

Run Python PreCommit

@pabloem
Copy link
Member

pabloem commented Dec 19, 2018

Thanks Kasia. Sorry that it was just the flakiness.

@pabloem pabloem merged commit 0c589db into apache:master Dec 19, 2018
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.

None yet

2 participants