Skip to content

Fixes TextIO and AvroIO tests of watchForNewFiles#3957

Closed
jkff wants to merge 1 commit intoapache:masterfrom
jkff:read-watch-test
Closed

Fixes TextIO and AvroIO tests of watchForNewFiles#3957
jkff wants to merge 1 commit intoapache:masterfrom
jkff:read-watch-test

Conversation

@jkff
Copy link
Copy Markdown
Contributor

@jkff jkff commented Oct 6, 2017

  • AvroIO: Need to specify a trigger to make sure that files are really generated continuously and testing of watchForNewFiles is non-vacuous.

  • TextIO: files were generated by manual code, and sometimes writing of a file could race with TextIO reading it, and it might see the same file with two different sizes, and count it as two different files (two Metadata objects for the same filename with different sizes are not equal) and read the file twice.

It makes sense to address that separately: e.g. in the Watch transform allow specifying a key extractor - but it's outside the scope of this PR and tracked in https://issues.apache.org/jira/browse/BEAM-3030.

R: @reuvenlax

* AvroIO: Need to specify a trigger to make sure that files are really
generated continuously and testing of watchForNewFiles is non-vacuous.

* TextIO: files were generated by manual code,
and sometimes writing of a file could race with TextIO reading it, and
it might see the same file with two different sizes, and count it as two
different files (two Metadata objects for the same filename with
different sizes are not equal) and read the file twice.

It makes sense to address that separately: e.g. in the Watch transform
allow specifying a key extractor - but it's outside the scope of this
PR.
@coveralls
Copy link
Copy Markdown

Coverage Status

Changes Unknown when pulling 59b450d on jkff:read-watch-test into ** on apache:master**.

@jkff
Copy link
Copy Markdown
Contributor Author

jkff commented Oct 7, 2017

retest this please

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.003%) to 69.587% when pulling 59b450d on jkff:read-watch-test into b82195a on apache:master.

@jbonofre
Copy link
Copy Markdown
Member

jbonofre commented Oct 8, 2017

It sounds good to me and should be merged pretty quickly as it impacts builds of other PRs.

@asfgit asfgit closed this in 5130cab Oct 9, 2017
@jkff jkff deleted the read-watch-test branch October 9, 2017 18:40
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.

3 participants