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-143] add test for UnboundedSourceWrapper #69

Closed
wants to merge 3 commits into from

Conversation

mxm
Copy link
Contributor

@mxm mxm commented Mar 23, 2016

So far the UnboundedSourceWrapper has not been tested systematically which led to a bunch of regression issues. This PR fixes the UnboundedSourceWrapper and adds a test case. The test case ensures that the wrapper works as expected. In particular, a full integration test is run to check whether serialization and instantiation of the wrapper works in a cluster setup.

mxm added 2 commits March 23, 2016 16:43
Now, we initialize the UnboundedSourceReader at runtime which requires
us to keep a copy of the PipelineOptions. This should be fine here
because we are at the lowest point of the execution stack.
The test ensures serialization and execution of the wrapper works as
expected.
@mxm
Copy link
Contributor Author

mxm commented Mar 23, 2016

CC @aljoscha @kl0u

*
*</p>
* For now we support non-parallel, not checkpointed sources.
* For now we only support non-parallel sources.
* */
public class UnboundedSourceWrapper<T> extends RichSourceFunction<WindowedValue<T>> implements Triggerable {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this is a wrapper for Read.Unbounded not UnboundedSource -- maybe should rename to UnboundedReadWrapper? (Or ReadUnboundedWrapper)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it really is a wrapper for UnboundedSource. In my view Read.Unbounded is just the API construct that itself contains an UnboundedSource. Said source is then handled by the UnboundedSourceWrapper at runtime.

@dhalperi
Copy link
Contributor

Peppered a few comments, but looks great to me.

Glad to have the chance to get my eyes on some Flink code!

@dhalperi
Copy link
Contributor

er, +1

@mxm
Copy link
Contributor Author

mxm commented Mar 23, 2016

Thanks for your comments @dhalperi @tgroh. I've made some improvements.

@asfgit asfgit closed this in 2f90258 Mar 23, 2016
iemejia referenced this pull request in iemejia/beam Jan 12, 2018
mareksimunek pushed a commit to mareksimunek/beam that referenced this pull request May 9, 2018
…riptors

apache#61 [euphoria-flink] Do not hold states in memory
ryucc added a commit to ryucc/beam that referenced this pull request Feb 1, 2023
pl04351820 pushed a commit to pl04351820/beam that referenced this pull request Dec 20, 2023
towards apache#65

this PR is staged on top of apache#68 as later refactor changes will be cross-class; I encourage reviewing in sequential order
pl04351820 pushed a commit to pl04351820/beam that referenced this pull request Dec 20, 2023
towards apache#65

this PR is staged on top of apache#69 as refactor changes are cross-class; I encourage reviewing in sequential order
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.

4 participants