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

Fixed bug where DataprocPysparkTask overrides the job name parameter … #1878

Merged
merged 1 commit into from
Oct 13, 2016

Conversation

matt-miller
Copy link
Contributor

Description

The name of the main pyspark script was hardcoded to be main_job.py, it now uses the provided parameter.

Have you tested this? If so, how?

I have made my own version of DataprocPysparkTask with this change to successfully run pyspark jobs on Dataproc, whereas the original version failed due to an inability to find main_job.py. Also note that there is a unit test for this that failed to catch the issue because the unit test happened to use main_job.py as its main job name.

@mention-bot
Copy link

@matt-miller, thanks for your PR! By analyzing the history of the files in this pull request, we identified @constantijn and @piotrpolatowski to be potential reviewers.

@dlstadther
Copy link
Collaborator

I don't use dataproc, but based on code lgtm.

@Tarrasch
Copy link
Contributor

Can you also fix the unit test?

@constantijn
Copy link

Change looks good to me ... not sure the unit test was broken, seems I forgot to replace a string (the job name) with the actual parameter passed in, and instead passed in a hard coded string of the job file i normally use. The test is there, it just didn't show up because the value I was using for the parameter was the same as the hard coded string i accidentally left in.

Speaking of tests ... do you have any idea why travis is reporting an error? It seems the running of the tests on some other part of the code broke

@matt-miller
Copy link
Contributor Author

matt-miller commented Oct 12, 2016

The unit test is fine, it just happened to not catch the bug. It shouldn't need any revision.

I have no idea what the Travis errors are all about. I distinctly remember master having a failed build when I forked the repo yesterday but looking back at the Travis builds I can't find any evidence to support this claim.

@Tarrasch Tarrasch merged commit c29aef6 into spotify:master Oct 13, 2016
Tarrasch added a commit that referenced this pull request Oct 13, 2016
There was a sporadic failure happening. Last time it bothered us in #1878.
Tarrasch added a commit that referenced this pull request Oct 21, 2016
There was a sporadic failure happening. Last time it bothered us in #1878.
This was referenced Jun 29, 2022
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.

5 participants