8000 [luigi.contrib.spark] tracking_url_pattern as a property by drowoseque · Pull Request #2820 · spotify/luigi · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[luigi.contrib.spark] tracking_url_pattern as a property #2820

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.

Sign up for GitHub

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
Merged
merged 1 commit into from
Nov 21, 2019
Merged

[luigi.contrib.spark] tracking_url_pattern as a property #2820

merged 1 commit into from
Nov 21, 2019

Conversation

drowoseque
Copy link
Contributor

Description

tracking_url_pattern setting took out from method run and was redefined as a property

Motivation and Context

It's normally no a very good practice to put a variable setting logic inside other method
So I took it out

Have you tested this? If so, how?

All tests are passed in test/contrib/spark_test.py
Particularly test_tracking_url_is_found_in_stderr_cluster_mode tests correctness of tracking_url_pattern work

@drowoseque
Copy link
Contributor Author

@honnix @Tarrasch @dlstadther could you please review this PR?

dlstadther
dlstadther requested changes Nov 16, 2019
View reviewed changes
Copy link
Collaborator
@dlstadther dlstadther left a comment

Choose a reason for hiding this comment

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

You've removed the run method and its call to super().run(). That doesn't seem desirable.

@drowoseque
Copy link
Contributor Author
drowoseque commented Nov 16, 2019 •
edited
Loading

@dlstadther

You've removed the run method and its call to super().run(). That doesn't seem desirable.

Why?
In previous variant method run was the same as its parent's one except the fact that tracking_url_pattern was set inside it.
I just put definition of tracking_url_pattern out of run method into a property so now we don't need to redefine run method in SparkSubmitTask, so the logic stays the same (test runs prove that), just code is little bit cleaner

dlstadther
dlstadther approved these changes Nov 16, 2019
View reviewed changes
Copy link
Collaborator
@dlstadther dlstadther left a comment

Choose a reason for hiding this comment

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

My bad - i see now. Thanks for clarifying!

@drowoseque
Copy link
Contributor Author

@dlstadther how many approves do i need for merging it?

@dlstadther dlstadther merged commit dc2e57f into spotify:master Nov 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dlstadther dlstadther dlstadther approved these changes

@honnix honnix Awaiting requested review from honnix

@Tarrasch Tarrasch Awaiting requested review from Tarrasch

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

Successfully merging this pull request may close these issues.
2 participants
@drowoseque @dlstadther
0