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

Jump to bottom

[BEAM-2314] Add ValidatesRunner test for merging custom windows #3286

Closed
wants to merge 1 commit into from
Closed

[BEAM-2314] Add ValidatesRunner test for merging custom windows #3286

wants to merge 1 commit into from

Conversation

echauchot
Copy link
Contributor

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

  • Make sure the PR title is formatted like:
    [BEAM-<Jira issue #>] Description of pull request
  • Make sure tests pass via mvn clean verify.
  • Replace <Jira issue #> in the title with the actual Jira issue
    number, if there is one.
  • If this contribution is large, please file an Apache
    Individual Contributor License Agreement.

@aljoscha I reopened a new PR to test that jenkins integration is working again as requested
R: @aljoscha
CC: @jbonofre

@echauchot echauchot mentioned this pull request Jun 2, 2017
Closed
4 tasks
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 70.682% when pulling 3be3911 on echauchot:BEAM-2314_merging_windows_test into 7a075cc on apache:master.

@aljoscha
Copy link
Contributor

aljoscha commented Jun 4, 2017

Pre-commit seems to be failing.

@aljoscha
Copy link
Contributor

aljoscha commented Jun 4, 2017

Run Flink ValidatesRunner

@aljoscha
Copy link
Contributor

aljoscha commented Jun 4, 2017

Run Spark ValidatesRunner

@aljoscha
Copy link
Contributor

aljoscha commented Jun 4, 2017

Run Apex ValidatesRunner

@aljoscha
Copy link
Contributor

aljoscha commented Jun 4, 2017

Run Gearpump ValidatesRunner

@aljoscha
Copy link
Contributor

aljoscha commented Jun 4, 2017

retest this please

@aljoscha
Copy link
Contributor

aljoscha commented Jun 4, 2017

@echauchot I'm afraid you have to introduce an exclusion (similar to UsesTestStream or UsesMapState) because at least the Spark runner doesn't handle custom merging windows well. Did you see that problem also in the NEXMark tests?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 70.65% when pulling 3be3911 on echauchot:BEAM-2314_merging_windows_test into 7a075cc on apache:master.

@echauchot
Copy link
Contributor Author

echauchot commented Jun 5, 2017 •
edited

@aljoscha: strange, the pre-commit tests were passing and I changed nothing, I'll dig into it, maybe my test is flaky. For your question about Spark runner: it runs NexMark query9 (custom window merging) correctly in both batch and streaming modes. Maybe there is something different from Nexmark query in my ValidatesRunner test. Once again, I'll take a look and ping you when it's ready

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 70.638% when pulling 4008bca on echauchot:BEAM-2314_merging_windows_test into 7a075cc on apache:master.

@echauchot
Copy link
Contributor Author

Run Spark ValidatesRunner

@aljoscha
Copy link
Contributor

aljoscha commented Jun 8, 2017

The spark tests still seem to be failing. Any news?

Maybe @aviemzur has an idea what could be going on?

@echauchot
Copy link
Contributor Author

echauchot commented Jun 8, 2017 •
edited

Hi @aljoscha, I have switched to other ongoing things. I think it is related to the way I use the custom coder that generates a class cast. I'll discuss with Aviem about that particular point.

@echauchot
Copy link
Contributor Author

Run Apex ValidatesRunner

@echauchot
Copy link
Contributor Author

Run Gearpump ValidatesRunner

@echauchot
Copy link
Contributor Author

Run Flink ValidatesRunner

@echauchot
Copy link
Contributor Author

Seems that after all the coder is not the problem: the problem is that Spark runner casts the CustomWindow to IntervalWindow in the merge process, hence the serialization issue with the CustomWindow downstream. I'm investigating how to fix that up.

@jbonofre
Copy link
Member

Gonna take a look.

R: @jbonofre

@echauchot
Copy link
Contributor Author

I opened an issue on the spark runner side to fix custom window merging (https://issues.apache.org/jira/browse/BEAM-2499)
R: @amitsela I add you as a reviewer on this PR so that you can give your opinion on whether this ValidatesRunner test is valid.
@aljoscha, @amitsela, maybe we could merge this PR and use this new test to ensure that all the runners support Custom Window merging?

@echauchot
Copy link
Contributor Author

@aljoscha @amitsela WDYT?

@aljoscha
Copy link
Contributor

From my side the test looks good. We could think about adding it to WindowTest, instead of creating a new file, however.

Regarding the Spark runner, this needs either an exclusion or a fix.

@echauchot
Copy link
Contributor Author

Thanks @aljoscha. I agree, it is better to put this test with the other window related tests. I'll also add a new category interface to skip this test in runners that don't support it

@echauchot echauchot force-pushed the BEAM-2314_merging_windows_test branch from 4008bca to 40507b9 Compare June 30, 2017 07:47
@echauchot
Copy link
Contributor Author

Done and rebased on master

aljoscha
aljoscha reviewed Jun 30, 2017
View reviewed changes
sdks/java/core/src/main/java/org/apache/beam/sdk/testing/UsesCustomWindowMerging.java Outdated
import org.apache.beam.sdk.transforms.ParDo;

/**
* Category tag for validation tests which utilize splittable {@link ParDo}.
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 this comment was not properly adapted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, bad copy/paste :) Thanks for pointing !

@aljoscha
Copy link
Contributor

Run Flink ValidatesRunner

@aljoscha
Copy link
Contributor

Run Spark ValidatesRunner

@aljoscha
Copy link
Contributor

retest this please

@aljoscha
Copy link
Contributor

Run Flink ValidatesRunner

@echauchot
Copy link
Contributor Author

Spark Runner Validates runner failed with connection reset on https://repo.maven.apache.org:443

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 70.868% when pulling cbbc552 on echauchot:BEAM-2314_merging_windows_test into 893bf42 on apache:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 70.868% when pulling cbbc552 on echauchot:BEAM-2314_merging_windows_test into 893bf42 on apache:master.

@echauchot
Copy link
Contributor Author

Run Spark ValidatesRunner

@echauchot echauchot mentioned this pull request Jul 7, 2017
Closed
4 tasks
@echauchot
Copy link
Contributor Author

echauchot commented Jul 7, 2017 •
edited

@aljoscha @amitsela Please note that this PR is to be merged before #3517 because it adds the test that is used in #3517
#3517 fixes custom window merging in Spark Runner

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 70.86% when pulling 424f958 on echauchot:BEAM-2314_merging_windows_test into 893bf42 on apache:master.

@echauchot
Copy link
Contributor Author

Run Spark ValidatesRunner

@echauchot
Copy link
Contributor Author

Spark Runner ValidatesRunner tests errors above are not related to this PR, they are not related to custom window merging. I think we could merge the PR if you are ok.
WDYT?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 70.847% when pulling 165361d on echauchot:BEAM-2314_merging_windows_test into 893bf42 on apache:master.

@echauchot
Copy link
Contributor Author

Run Spark ValidatesRunner

@echauchot
Copy link
Contributor Author

Run Flink ValidatesRunner

1 similar comment
@echauchot
Copy link
Contributor Author

Run Flink ValidatesRunner

@echauchot echauchot mentioned this pull request Jul 19, 2017
Closed
6 tasks
@amitsela
Copy link
Member

LGTM on excluding SparkRunner

@echauchot echauchot force-pushed the BEAM-2314_merging_windows_test branch from 165361d to 9e638d0 Compare July 24, 2017 09:06
@echauchot
Copy link
Contributor Author

echauchot commented Jul 24, 2017 •
edited

Above validatesRunner tests failures are not related to this PR. See these tickets:
https://issues.apache.org/jira/browse/BEAM-2670
https://issues.apache.org/jira/browse/BEAM-2671

@echauchot
Copy link
Contributor Author

R: @aviemzur

@echauchot
Copy link
Contributor Author

Run Flink ValidatesRunner

@echauchot
Copy link
Contributor Author

Run Spark ValidatesRunner

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 70.596% when pulling 9e638d0 on echauchot:BEAM-2314_merging_windows_test into f54072a on apache:master.

@asfgit asfgit closed this in 0064fb3 Jul 24, 2017
@echauchot echauchot deleted the BEAM-2314_merging_windows_test branch July 24, 2017 15:27
@aviemzur
Copy link
Member

Merged based on @amitsela LGTM

@kennknowles kennknowles mentioned this pull request Jun 3, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aljoscha aljoscha aljoscha left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@echauchot @coveralls @aljoscha @jbonofre @amitsela @aviemzur