-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-8528] Expose SparkContext.applicationId in PySpark #6936
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
|
The motivation for this change seems fine to me, although I have some nits to pick with the format of the docstrings. |
python/pyspark/context.py
Outdated
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 would try to make this docstring match the style of our other docstrings a bit better. Specifically, there's not a need to say applicationId - here, since that's clear from the method's name.
Instead, I'd have this first line just read something like this:
A unique identifier for the Spark application.
Its format depends on the scheduler implementation.
I'm not sure whether we need to show the examples here, but I guess it doesn't necessarily hurt.
|
Also, do you mind updating the PR title so that it's not truncated, e.g. "[SPARK-8528] Expose SparkContext.applicationId in PySpark"? |
|
Done updating docstrings. Thank you for suggestions. |
|
Jenkins, this is ok to test. |
|
Test build #35561 timed out for PR 6936 at commit |
|
Interesting build results - not tests failed, but whole build timeouted |
|
Jenkins, please test it again |
|
Jenkins, retest this please. |
|
Test build #35815 has finished for PR 6936 at commit
|
|
Hm, looks like last test haven't honored ELLIPSIS. Anyway this is doctest is not particularly useful. The contents of the applicationId is set outside of the test - and can be changed anytime, i.e. if tests would run in SparkContext in yarn or remote cluster - the result will be different. So I'll simplify the test - it will just check that such property exists and returns something |
|
Please test updated version - I made the doctest stable |
|
Test build #35861 has finished for PR 6936 at commit
|
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.
Minor nit: the u' is going to be confusing to Scala users. I'd just go with regular quoted strings here.
|
BTW, I think that you can get this to work with doctest.ELLIPSIS; the problem is likely Python 2.x vs 3.x differences. You can fix this by using the |
|
Thanks for hints |
|
Yeah, no rush; sorry for the hassle. At some point we should update the contributing guide to include tips on this stuff, such as the doctest trick. |
|
Python doctests are rather limited in expressiveness, but efficient in keeping examples bug free in documentation. |
|
Test build #35905 has finished for PR 6936 at commit
|
|
LGTM, so I'm going to merge this to master. Thanks! |
Use case - we want to log applicationId (YARN in hour case) to request help with troubleshooting from the DevOps