-
Notifications
You must be signed in to change notification settings - Fork 4.5k
[BEAM-8830] Fix flatten in the new spark runner #10293
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
Conversation
aromanenko-dev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM, just a minor question (see inline).
Also, please squash all commits before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to have DEBUG log level in tests by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in test I think so because it prints Beam DAG and spark DAG
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid to have too much logs for all Spark tests that could consume too much disk space on Jenkins servers (since Spark is very verbose).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well it is just for the runner not for the spark engine. And the runner puts so few logs (I think only the DAGs).
But anyway, it appears that for some reason, to have the debug logs even in UTests, it requires to change the production conf log file! Spark has always been quite difficult to set up with log4j. Maybe it is due to the test file named log4j-test.properties. Anyway I'll put it back to error until we figure out how to configure it properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same question as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same answer than above :)
|
@aromanenko-dev thanks for the prompt review ! |
33ae949 to
41f3aa6
Compare
|
done |
|
Run Spark StructuredStreaming ValidatesRunner |
|
Some ValidatesRunner tests started to fail even if they passed before |
…at sets the schema to binary. Remove test exclusions for now passing tests Set test log level to debug
41f3aa6 to
62bd3ff
Compare
Strange because it does not use emptyDataset (the only thing that has changed). It might be a side effect, I'll take a look |
The test passes on my machine. Maybe an unrelated flaky test |
|
Run Spark StructuredStreaming ValidatesRunner |
|
@aromanenko-dev : OOM on last run of VR tests. one splittableDoFn test is taking 5 min I guess it is not normal. But anyway, SDF is not implemented on the runner. I think we should exclude all SDF tests. WDYT ? |
7b6f604 to
7dd8354
Compare
|
@aromanenko-dev with @iemejia last comment I just reduced the SDF tests exclusion scope to only unbounded tests and |
7dd8354 to
f67bc41
Compare
|
Run Spark StructuredStreaming ValidatesRunner |
|
Run Java PreCommit |
|
@aromanenko-dev timeout in un unrelated portability test. |
|
Run Java PreCommit |
|
@aromanenko-dev all the tests pass, as you gave LGTM, I'll auto-merge. Thanks for the review ! |
f67bc41 to
a9a68a8
Compare
|
Run Spark StructuredStreaming ValidatesRunner |
|
just reworded the last commit message |
|
Run Java PreCommit |
R: @aromanenko-dev
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.See the Contributor Guide for more tips on how to make review process smoother.
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.