[MINOR][PYTHON] 2 Improvements to Pyspark docs #21057

aviv-ebates · 2018-04-12T17:56:30Z

Each of these 2 items has cost me a few hours of debugging. Hopefully, this will stop others from having to debug the same thing.

Describe the expected type of fromOffsets param.
Add missing method to StreamingListener, which is invoked by proxy from the Java/Scala side, and throws a strange exception when not found.

HyukjinKwon · 2018-04-13T06:53:07Z

python/pyspark/streaming/kafka.py

I would say sth like a dictionary containing `TopicAndPartition` to integers..

Yeah, that works too.

Shall we fix as so?

HyukjinKwon · 2018-04-14T14:10:52Z

ok to test

HyukjinKwon · 2018-04-14T14:11:49Z

BTW, mind fixing the PR title to [MINOR][PYTHON] ... and make the title more descriptive? not a big deal but good to match it with other PRs.

SparkQA · 2018-04-14T14:14:15Z

Test build #89368 has finished for PR 21057 at commit e040a68.

This patch fails Python style tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2018-04-15T02:01:47Z

python/pyspark/streaming/listener.py

Can you add a test to pyspark.streaming.tests.StreamingListenerTests?

this isnt doc only change then, I think?

I first thought it's doc only change but I realised it's actually not after taking a close look.

Implementing onStreamingStarted looks actually required:

Add missing method to StreamingListener, which is invoked by proxy from the Java/Scala side, and throws a strange exception when not found.

This wasn't there at the first - https://github.com/apache/spark/blob/ace0db47141ffd457c2091751038fc291f6d5a8b/python/pyspark/streaming/listener.py / https://github.com/apache/spark/blob/ace0db47141ffd457c2091751038fc291f6d5a8b/streaming/src/main/scala/org/apache/spark/streaming/api/java/JavaStreamingListener.scala ; however, this method was added from ce99f51 but seems Python's implementation is missed.

Strictly it sounds better to have an explicit test since @aviv-ebates has a reproducer (assuming from the description) and should be easy to add.

viirya · 2018-04-15T02:02:53Z

I think you may create a minor JIRA ticket for this.

HyukjinKwon · 2018-04-15T02:26:58Z

+1 for ^.

Since this method is missing here, trying to use a Listener throws an expection. Since it's not documented, it's hard to handle.

SparkQA · 2018-04-16T15:19:14Z

Test build #89399 has finished for PR 21057 at commit 010de10.

This patch fails Python style tests.
This patch merges cleanly.
This patch adds no public classes.

aviv-ebates · 2018-04-16T15:26:06Z

Updated title
Updated text to one suggested by @HyukjinKwon
I don't have a reasonable way to make a test. Test scenario is "Configure pythong listener, run actual spark streaming". If you have a way to test that, that would be great; I don't know how to set up this test.
Feel free to create a ticket and update commit messages when you merge this.
I consider this a documentation change, because the method is already available and required; All I did is make sure it shows up in documentation (and incidentally, make it not-required). Feel free to change commit messages and pr text to reflect your understanding of this change.

HyukjinKwon · 2018-04-17T14:19:13Z

It would be helpful if you open a JIRA and describe the issue. It could help other guys think a better way to test or would give clearer ideas to see if it's really difficult to add a test. Usually, JIRA is made first. See also https://spark.apache.org/contributing.html.

aviv-ebates · 2018-04-17T16:41:20Z

If you're not clear about what I've done here, ask away. I don't wish to create a jira account in addition; Feel free to create whatever tickets you think are required.
I've already paid my price for this particular deficiency in Spark documentation, so I don't need this docfix for myself. I was hoping you'd like to improve the docs, so that the next guy would have an easier experience using Spark/Pyspark, but that's totally up to you.
I think I've done all that is reasonable to do for this documents improvements for your project. If you want this change, merge it in; If you don't, don't.

HyukjinKwon · 2018-04-18T00:13:43Z

Right, let me try to cherry-pick and see if I can write a test. Will try to have some time and open a PR after cherry-picking your commit. I think you can close this PR.

HyukjinKwon · 2018-04-18T01:02:21Z

Actually @viirya, would you be interested in this if you are available? I will do this by myself but I am currently not quite available. If you are busy too, let me try it later anyway.

viirya · 2018-04-18T01:06:12Z

I will flight to Korea for a company workshop today. I can do this maybe only in at tonight. If this isn't urgent, then it is okay.

…

On Wed, Apr 18, 2018, 10:03 AM Hyukjin Kwon ***@***.***> wrote: Actually @viirya <https://github.com/viirya>, would you be interested in this if you are available? I will do this by myself but I am currently not quite available. If you are busy too, let me try it anyway. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#21057 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEM914hLAgFHKhwMS9OU_PVN7G-WvQ9ks5tppDmgaJpZM4TSO_J> .

HyukjinKwon · 2018-04-18T01:12:47Z

It's not urgent at all. I would appreciate it.

HyukjinKwon · 2018-04-18T01:13:59Z

( I regret I happened to come over form Korea to Singapore too fast before your flight :-) )

viirya · 2018-04-18T14:13:15Z

:-)

…ener ## What changes were proposed in this pull request? The `StreamingListener` in PySpark side seems to be lack of `onStreamingStarted` method. This patch adds it and a test for it. This patch also includes a trivial doc improvement for `createDirectStream`. Original PR is #21057. ## How was this patch tested? Added test. Author: Liang-Chi Hsieh <[email protected]> Closes #21098 from viirya/SPARK-24014. (cherry picked from commit 8bb0df2) Signed-off-by: jerryshao <[email protected]>

…ener ## What changes were proposed in this pull request? The `StreamingListener` in PySpark side seems to be lack of `onStreamingStarted` method. This patch adds it and a test for it. This patch also includes a trivial doc improvement for `createDirectStream`. Original PR is apache#21057. ## How was this patch tested? Added test. Author: Liang-Chi Hsieh <[email protected]> Closes apache#21098 from viirya/SPARK-24014.

HyukjinKwon approved these changes Apr 13, 2018

View reviewed changes

viirya reviewed Apr 15, 2018

View reviewed changes

aviv-ebates changed the title ~~2 Improvements to Pyspark docs~~ [MINOR][PYTHON] 2 Improvements to Pyspark docs Apr 16, 2018

aviv-ebates added 2 commits April 16, 2018 08:16

improve doc1

67cef00

missing, undocumented, method.

010de10

Since this method is missing here, trying to use a Listener throws an expection. Since it's not documented, it's hard to handle.

aviv-ebates closed this Apr 18, 2018

viirya mentioned this pull request Apr 18, 2018

[SPARK-24014][PySpark] Add onStreamingStarted method to StreamingListener #21098

Closed

[MINOR][PYTHON] 2 Improvements to Pyspark docs #21057

[MINOR][PYTHON] 2 Improvements to Pyspark docs #21057

Uh oh!

Conversation

aviv-ebates commented Apr 12, 2018

Uh oh!

HyukjinKwon Apr 13, 2018

Choose a reason for hiding this comment

Uh oh!

aviv-ebates Apr 13, 2018

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Apr 14, 2018

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Apr 14, 2018

Uh oh!

HyukjinKwon commented Apr 14, 2018

Uh oh!

SparkQA commented Apr 14, 2018

Uh oh!

viirya Apr 15, 2018

Choose a reason for hiding this comment

Uh oh!

felixcheung Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Apr 16, 2018

Choose a reason for hiding this comment

Uh oh!

viirya commented Apr 15, 2018

Uh oh!

HyukjinKwon commented Apr 15, 2018

Uh oh!

SparkQA commented Apr 16, 2018

Uh oh!

aviv-ebates commented Apr 16, 2018

Uh oh!

HyukjinKwon commented Apr 17, 2018

Uh oh!

aviv-ebates commented Apr 17, 2018

Uh oh!

HyukjinKwon commented Apr 18, 2018

Uh oh!

HyukjinKwon commented Apr 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

viirya commented Apr 18, 2018 via email

Uh oh!

HyukjinKwon commented Apr 18, 2018

Uh oh!

HyukjinKwon commented Apr 18, 2018

Uh oh!

viirya commented Apr 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HyukjinKwon commented Apr 18, 2018 •

edited

Loading