[BEAM-215] Implement Create as An OffsetBasedSource #183
Conversation
tgroh
force-pushed
the
create_as_a_source_always
branch
from
2059a01 to
54a5d1d
Compare
|
R: @dhalperi |
tgroh
force-pushed
the
create_as_a_source_always
branch
from
54a5d1d to
1fd3c88
Compare
|
@tgroh not passing, will move on to other review ;) |
tgroh
force-pushed
the
create_as_a_source_always
branch
from
1fd3c88 to
87883f1
Compare
This removes the requirement to implement a primtiive Create for runners that support Reads from a Bounded Source. Remove Dataflow Runner references to Create.Values
tgroh
force-pushed
the
create_as_a_source_always
branch
from
87883f1 to
9e154c4
Compare
|
Tests are passing with #214 in the history. PTAL. |
tgroh
changed the title
| @Override | ||
| protected boolean advanceImpl() throws IOException { | ||
| CreateSource<T> source = getCurrentSource(); | ||
| index++; |
There was a problem hiding this comment.
set next to null here? So that getCurrent() throws the proper NoSuchElelementException.
tgroh
force-pushed
the
create_as_a_source_always
branch
from
ae278d3 to
87fc494
Compare
| TestPipeline p = TestPipeline.create(); | ||
| PCollection<Record> unencodable = | ||
| p.apply(Create.of(new Record()).withCoder(new RecordNoEncodeCoder())); | ||
| p.apply(Create.<Record>of().withCoder(new RecordNoEncodeCoder())); |
There was a problem hiding this comment.
Can you do this more faithfully (or perhaps additionally) by adding a test with Create.of(t) -> ParDo.of(emit uncodable)?
There was a problem hiding this comment.
Maybe just refactor the common code to a utility method to make it clear what is being tested in this and below tests?
The key IIUC is that this is just a unit test of ModelEnforcement, and all of the setup code is just to get the ability to create bundles of the test record in the right way.
There was a problem hiding this comment.
Refactored the setup code into a common location, less the success case, due to the need to create a proper InProcessTransformResult instance.
Trailing space error :(
| /** | ||
| * Tests for {@link InProcessCreate}. | ||
| */ | ||
| @RunWith(JUnit4.class) |
There was a problem hiding this comment.
Are the tests for existing Create this good or do you need to re-add these under the new name?
There was a problem hiding this comment.
About half of them. Anything based on the underlying source, + serializable elements needed to be ported (and now has been)
|
LGTM, ping me when travis is green again and I can merge ;) |
tgroh
force-pushed
the
create_as_a_source_always
branch
from
ee876ae to
1c56762
Compare
Correct URL for subnetwork
added IOUtils to help iterate over methods throwing IOException
2 participants
Be sure to do all of the following to help us incorporate your contribution
quickly and easily:
[BEAM-<Jira issue #>] Description of pull requestmvn clean verify. (Even better, enableTravis-CI on your fork and ensure the whole test matrix passes).
<Jira issue #>in the title with the actual Jira issuenumber, if there is one.
Individual Contributor License Agreement.
This removes the requirement to implement a primtiive Create for runners
that support Reads from a Bounded Source.
Remove the override in the InProcessRunner
This is part of the new Beam Runner API, which removes Create as a primitive PTransform
https://docs.google.com/document/d/1bao-5B6uBuf-kwH1meenAuXXS0c9cBQ1B2J59I3FiyI/edit#heading=h.p6lvszfbmyj6