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

[SPARK-19584] [SS] [DOCS] update structured streaming documentation around batch mode #16918

Closed
wants to merge 7 commits into from

Conversation

tcondie
Copy link
Contributor

@tcondie tcondie commented Feb 14, 2017

What changes were proposed in this pull request?

Revision to structured-streaming-kafka-integration.md to reflect new Batch query specification and options.

@zsxwing @tdas

Please review http://spark.apache.org/contributing.html before opening a pull request.

@SparkQA
Copy link

SparkQA commented Feb 14, 2017

Test build #72840 has finished for PR 16918 at commit 081ea24.

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

@SparkQA
Copy link

SparkQA commented Feb 14, 2017

Test build #72839 has finished for PR 16918 at commit debe111.

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

Copy link
Member

@zsxwing zsxwing left a comment

Choose a reason for hiding this comment

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

Overall looks good. Left some nits

@@ -119,6 +119,122 @@ ds3.selectExpr("CAST(key AS STRING)", "CAST(value AS STRING)")
</div>
</div>

### Creating a Kafka Source Batch

Copy link
Member

Choose a reason for hiding this comment

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

It's better to add a sentence here before the codes, such as If you have a use case that is better suited to batch processing, you can create an Dataset/DataFrame for a defined range of offsets.

</td>
<td>latest</td>
<td>batch only</td>
<td>The end point when a batch query is started, either "latest" which is just from the latest
Copy link
Member

Choose a reason for hiding this comment

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

nit The end point when a batch query is started , either "latest" which is just from the latest -> The end point when a batch query is ended, either "latest" which is just referred to the latest

Copy link
Member

Choose a reason for hiding this comment

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

nit: a json string specifying a starting offset for each TopicPartition. -> a json string specifying an ending offset for each TopicPartition.

.format("kafka")
.option("kafka.bootstrap.servers", "host1:port1,host2:port2")
.option("subscribe", "topic1")
.load()
Copy link
Member

Choose a reason for hiding this comment

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

nit: missing ";"

.option("subscribe", "topic1,topic2")
.option("startingOffsets", "{\"topic1\":{\"0\":23,\"1\":-2},\"topic2\":{\"0\":-2}}")
.option("endingOffsets", "{\"topic1\":{\"0\":50,\"1\":-1},\"topic2\":{\"0\":-1}}")
.load()
Copy link
Member

Choose a reason for hiding this comment

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

nit: missing ";"

.option("startingOffsets", "{\"topic1\":{\"0\":23,\"1\":-2},\"topic2\":{\"0\":-2}}")
.option("endingOffsets", "{\"topic1\":{\"0\":50,\"1\":-1},\"topic2\":{\"0\":-1}}")
.load()
ds2.selectExpr("CAST(key AS STRING)", "CAST(value AS STRING)")
Copy link
Member

Choose a reason for hiding this comment

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

nit: missing ";"

{% highlight python %}

# Subscribe to 1 topic defaults to the earliest and latest offsets
ds1 = spark
Copy link
Member

Choose a reason for hiding this comment

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

You need to add \ to tell the Python compiler not starting a new line.

@SparkQA
Copy link

SparkQA commented Feb 14, 2017

Test build #72843 has finished for PR 16918 at commit 28af040.

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


# Subscribe to 1 topic defaults to the earliest and latest offsets
ds1 = spark \
.read
Copy link
Member

Choose a reason for hiding this comment

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

You need to all \ to all lines.

@SparkQA
Copy link

SparkQA commented Feb 14, 2017

Test build #72887 has finished for PR 16918 at commit f5318e0.

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

@zsxwing
Copy link
Member

zsxwing commented Feb 14, 2017

@tcondie There is a typo about fetchOffset.numRetries in this page: Number of times to retry before giving up **fatch** Kafka **latest** offsets. -> Number of times to retry before giving up **fetching** Kafka **** offsets. Could you also fix it since you are touching this file?

@@ -152,7 +270,7 @@ Each row in the source has the following schema:
</tr>
</table>

The following options must be set for the Kafka source.
The following options must be set for the Kafka source (streaming and batch).
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Kafka source for both batch and streaming queries.

@@ -187,50 +305,68 @@ The following options must be set for the Kafka source.
The following configurations are optional:

<table class="table">
<tr><th>Option</th><th>value</th><th>default</th><th>meaning</th></tr>
<tr><th>Option</th><th>value</th><th>default</th><th>mode</th><th>meaning</th></tr>
Copy link
Contributor

@tdas tdas Feb 14, 2017

Choose a reason for hiding this comment

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

rather than "mode", how about "query type". just the term "mode" does not mean much, but "query type" says something.

<tr>
<td>startingOffsets</td>
<td>earliest, latest, or json string
<td>earliest, latest (streaming only), or json string
Copy link
Contributor

Choose a reason for hiding this comment

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

can you put the possible values in quotes. e.g.

"earliest", "latest" (only for streaming) or json string """  {"topicA":{"0":23,"1":-1},"topicB":{"0":-2}} """

<td>The start point when a query is started, either "earliest" which is from the earliest offsets,
"latest" which is just from the latest offsets, or a json string specifying a starting offset for
each TopicPartition. In the json, -2 as an offset can be used to refer to earliest, -1 to latest.
Note: This only applies when a new Streaming query is started, and that resuming will always pick
up from where the query left off. Newly discovered partitions during a query will start at
Note: For Batch, latest (either implicitly or by using -1 in json) is not allowed.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: For batch queries...

{"topicA":{"0":23,"1":-1},"topicB":{"0":-2}}
</td>
<td>latest</td>
<td>streaming=latest, batch=earliest</td>
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above, put the value quotes. its clearer if
"latest" for streaming,
"earliest" for batch

Note: This only applies when a new Streaming query is started, and that resuming will always pick
up from where the query left off. Newly discovered partitions during a query will start at
Note: For Batch, latest (either implicitly or by using -1 in json) is not allowed.
For Streaming, this only applies when a new Streaming query is started, and that resuming will
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: For streaming queries, this only ... when a new query is stasrted

@SparkQA
Copy link

SparkQA commented Feb 14, 2017

Test build #72895 has finished for PR 16918 at commit 5bf32f4.

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

<td>failOnDataLoss</td>
<td>true or false</td>
<td>true</td>
<td>Whether to fail the query when it's possible that data is lost (e.g., topics are deleted, or
<td>streaming only</td>
<td>Whether to fail the query when it's possible that data is lost (e.g., topics are deleted, or
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the streaming query
also could you add, what is the behavior for batch queries? that is the batch query will always fail if it fails to read any data.

<td>The start point when a query is started, either "earliest" which is from the earliest offsets,
"latest" which is just from the latest offsets, or a json string specifying a starting offset for
each TopicPartition. In the json, -2 as an offset can be used to refer to earliest, -1 to latest.
Note: This only applies when a new Streaming query is started, and that resuming will always pick
up from where the query left off. Newly discovered partitions during a query will start at
Note: For Batch queries, latest (either implicitly or by using -1 in json) is not allowed.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: why is B and S in Batch queries and Streaming queries randomly caps? I dont think "batch query" is a proper name of any API, and neither is "streaming query", unless you are referring the specific class "StreamingQuery".

Copy link
Contributor Author

@tcondie tcondie Feb 14, 2017

Choose a reason for hiding this comment

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

Agreed. The previous version had "S", so I wasn't sure if this was a convention or not. I'll make the correction.

@tdas
Copy link
Contributor

tdas commented Feb 14, 2017

@tcondie this is close and very good. just one more nit. otherwise LGTM.

@SparkQA
Copy link

SparkQA commented Feb 14, 2017

Test build #72904 has finished for PR 16918 at commit 8983b8d.

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

@SparkQA
Copy link

SparkQA commented Feb 15, 2017

Test build #72907 has finished for PR 16918 at commit d55ba09.

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

@tdas
Copy link
Contributor

tdas commented Feb 15, 2017

LGTM. merging now.

asfgit pushed a commit that referenced this pull request Feb 15, 2017
…und batch mode

## What changes were proposed in this pull request?

Revision to structured-streaming-kafka-integration.md to reflect new Batch query specification and options.

zsxwing tdas

Please review http://spark.apache.org/contributing.html before opening a pull request.

Author: Tyson Condie <[email protected]>

Closes #16918 from tcondie/kafka-docs.

(cherry picked from commit 447b2b5)
Signed-off-by: Tathagata Das <[email protected]>
@asfgit asfgit closed this in 447b2b5 Feb 15, 2017
cmonkey pushed a commit to cmonkey/spark that referenced this pull request Feb 15, 2017
…und batch mode

## What changes were proposed in this pull request?

Revision to structured-streaming-kafka-integration.md to reflect new Batch query specification and options.

zsxwing tdas

Please review http://spark.apache.org/contributing.html before opening a pull request.

Author: Tyson Condie <[email protected]>

Closes apache#16918 from tcondie/kafka-docs.
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.

4 participants