Skip to content

Conversation

@yssharma
Copy link

What changes were proposed in this pull request?

The examples and docs for Spark-Kinesis integrations use the deprecated KinesisUtils. We should update the docs to use the KinesisInputDStream builder to create DStreams.

How was this patch tested?

The patch primarily updates the documents. The patch will also need to make changes to the Spark-Kinesis examples. The examples need to be tested.

@SparkQA
Copy link

SparkQA commented May 23, 2017

Test build #77250 has finished for PR 18071 at commit 1e7e879.

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

@yssharma
Copy link
Author

Hey @budde , drawing your attention here please.
Just wanted to update the docs with the new Builder interface by you. Few questions on top of my mind -

  • Is it compatible with the python API as well like KinesisUtils used to be ?
  • How can I test that on the py spark-shell. Trying to use KinesisInputDStream gives me Builder not found execptions. I might be missing something obvious here.

@HyukjinKwon
Copy link
Member

Hi @yssharma, is it still WIP? I think we should make the PR on the mergable state at least.

@yssharma
Copy link
Author

@HyukjinKwon The PR is ready. Just waiting for some 👍 from the reviewers.

@yssharma yssharma changed the title [SPARK-20855][WIP][DStream] Update the Spark kinesis docs to use the KinesisInputDStream builder instead of deprecated KinesisUtils [SPARK-20855][Docs][DStream] Update the Spark kinesis docs to use the KinesisInputDStream builder instead of deprecated KinesisUtils Jul 24, 2017
@HyukjinKwon
Copy link
Member

It looks the last test was failed by style checking. Would you have some time to fix them up?

@yssharma
Copy link
Author

I noticed that now. Yes I will post an updated patch today. Thanks @HyukjinKwon

@yssharma yssharma force-pushed the ysharma/kinesis_docs branch from 1e7e879 to 9d50bd9 Compare July 24, 2017 10:24
@SparkQA
Copy link

SparkQA commented Jul 24, 2017

Test build #79904 has finished for PR 18071 at commit 9d50bd9.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@yssharma
Copy link
Author

Updated the patch @HyukjinKwon . Thanks for the note. 👍

@srowen
Copy link
Member

srowen commented Jul 24, 2017

I'm OK to merge something like this but don't know about the kinesis integration, not enough to review the content. Who could give it a second look, @budde ?

Copy link

@budde budde left a comment

Choose a reason for hiding this comment

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

LGTM overall. There are slight inconsistencies in the indent level between the old code samples in streaming-kinesis-integration.md and the new one but I'm not too familiar with how this gets rendered so perhaps this doesn't make a difference. Thanks for picking up my slack and updating the docs BTW— I should've really gotten to this when I added the builder interface.

@yssharma
Copy link
Author

Thanks @budde @srowen

@srowen
Copy link
Member

srowen commented Jul 25, 2017

Merged to master

@asfgit asfgit closed this in 4f77c06 Jul 25, 2017
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.

6 participants