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-5978] Changing parallelism for wordcount to 1 #7174

Merged
merged 1 commit into from Dec 13, 2018

Conversation

angoenka
Copy link
Contributor

Local execution environment sets the parallelism of the pipeline same as the number of cores on the machine. In my case, its 12. As we have 12 threads in SDKHarness, we get a deadlock on my machine.
Running on a cluster does not have this issue as the default parallelism there is 1.

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

It will help us expedite review of your Pull Request if you tag someone (e.g. @username) to look at it.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- --- --- --- ---
Java Build Status Build Status Build Status Build Status Build Status Build Status Build Status Build Status
Python Build Status --- Build Status
Build Status
Build Status --- --- ---

@angoenka angoenka requested review from mxm and tweise November 30, 2018 21:41
Copy link
Contributor

@tweise tweise left a comment

Choose a reason for hiding this comment

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

Please see #6954 (comment)

This is a problem specific to batch mode. Please set the workaround accordingly (there is already another one below) and add a comment pointing to the JIRA.

@tweise tweise changed the title [BEAM-5978] Changing parallalim for wordcount to 1 [BEAM-5978] Changing parallelism for wordcount to 1 Nov 30, 2018
@angoenka
Copy link
Contributor Author

angoenka commented Dec 1, 2018

Thanks. I will apply this parameter to batch only then.

@angoenka
Copy link
Contributor Author

angoenka commented Dec 1, 2018

Updated!

Copy link
Contributor

@tweise tweise left a comment

Choose a reason for hiding this comment

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

Let's wait for @mxm to take a look as well. I experimented a bit with the parallelism settings and documented it on https://issues.apache.org/jira/browse/BEAM-5167

Even though not needed for streaming currently, it is generally advisable to be explicit about the parallelism (predictability WRT network buffers etc.)

@tweise
Copy link
Contributor

tweise commented Dec 2, 2018

We could also fold this change into #7180

Unrelated to this change, note that currently wordcount doesn't work on macOS with the docker jobserver:

./gradlew :beam-sdks-python:portableWordCount
...
INFO:root:Using latest locally built Python SDK docker image.
docker: Error response from daemon: driver failed programming external connectivity on endpoint cranky_pasteur (f9b3843dada38679d655f258ca40af36b030b469f2abbd2277db199683018b53):  (iptables failed: iptables --wait -t nat -A DOCKER -p tcp -d 0/0 --dport 32817 -j DNAT --to-destination 172.17.0.2:0 ! -i docker0: iptables v1.6.2: Port `0' not valid

Try `iptables -h' or 'iptables --help' for more information.
 (exit status 2)).

Copy link
Contributor

@mxm mxm left a comment

Choose a reason for hiding this comment

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

As we have 12 threads in SDKHarness, we get a deadlock on my machine

Default should be 1. You would get auto-set 12 threads with it set to 0.

IMHO ok to merge this as a fix for as long as the underlying issue is still pending.

@angoenka
Copy link
Contributor Author

angoenka commented Dec 3, 2018

@mxm The underlying issue of flink and sdkHarness scheduling is still pending and requires more thought and discussion.

@tweise Mac wordcount should be working with #7178 Let me know if its otherwise.

@angoenka angoenka force-pushed the wc_change_parallalism branch 2 times, most recently from 1a2bb38 to 538a792 Compare December 3, 2018 20:12
@angoenka
Copy link
Contributor Author

angoenka commented Dec 3, 2018

Shall we merge this PR?

@mxm
Copy link
Contributor

mxm commented Dec 3, 2018

Go ahead :)

]
if (streaming)
options += ["--streaming"]
else
// workaround for local file output in docker container
options += ["--environment_cache_millis=10000"]
// [BEAM-5167] Workaround for scheduling issue between SDKHarness and Flink
options += ["--parallelism=1"]
Copy link
Contributor

Choose a reason for hiding this comment

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

if the intention is to use it only for batch, you would need to add {..}

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. That's why it's better to always use brackets for if blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, Totally missed it. BTW python works with indentation so it might have worked with Python :)

Should also apply this change to streaming as we don't want parallelism in streaming as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't make this more restrictive than necessary. So only batch for now.

@tweise
Copy link
Contributor

tweise commented Dec 3, 2018

@angoenka the error posted above applies to Docker execution (the default), as shown in the command.

@angoenka
Copy link
Contributor Author

Updated the PR

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.

None yet

3 participants