[BEAM-9650] Cleanup documentation on side inputs patterns #11415

Ardagan · 2020-04-14T18:10:53Z

[Website staging preview]

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang	SDK	Apex	Dataflow	Gearpump	Samza
Go		---	---	---	---
Java
Python		---		---	---
XLang	---	---	---	---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website
Non-portable
Portable	---		---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

Ardagan · 2020-04-14T18:11:10Z

R: @soyrice @rosetn

Ardagan · 2020-04-14T18:47:46Z

website/src/documentation/patterns/side-inputs.md

-You can read side input pcollection periodically into distinct windows.
-Later, when you apply side input to your main input, windows will be matched automatically 1:1.
-This way, you can guarantee side input consistency on the duration of the single window.
+You can read side input data periodically into distinct PCollection windows.


In Beam, we have IO.Read transforms that do read source data. Later data read can be assigned to specific window.

soyrice

Thanks for opening this PR :)

website/src/documentation/patterns/side-inputs.md

soyrice · 2020-04-14T20:58:53Z

website/src/documentation/patterns/side-inputs.md

+meaning that each window on main input side will be matched to a single
+version of side input data.
+
+To do this, you can utilize a combination of PeriodicSequence/PeriodicImpulse


It's probably best to replace "To do this" with "To do ABC" - it'll help the reader figure out what "this" refers to. Otherwise, if the reader is just skimming the page and starts at this paragraph, they have to read the previous paragraph to figure out what the pronoun refers to.

website/src/documentation/patterns/side-inputs.md

soyrice

Looks good. Just a couple more small comments to clear up a few parts of the doc.

website/src/documentation/patterns/side-inputs.md

soyrice · 2020-04-15T15:43:05Z

website/src/documentation/patterns/side-inputs.md


-To do this, you can utilize PeriodicSequence PTransform that will generate infinite sequence
-of elements with some real-time period:
+Described approach can be implemented using combination of


How about something like this: "To read side input data periodically... use a combination of PeriodicSequence..."

I'm a bit confused here since adding text you suggest is pretty much re-writing same thing that is a header and previous abstract. In next iteration I tried to remove this abstract overall since list of steps provides same information.

Gotcha - the intent is to summarize the previous abstract, so that it's easier for readers to skim the doc.

soyrice · 2020-04-15T15:45:26Z

website/src/documentation/patterns/side-inputs.md

-To do this, you can utilize PeriodicSequence PTransform that will generate infinite sequence
-of elements with some real-time period:
+Described approach can be implemented using combination of
+PeriodicSequence or PeriodicImpulse PTransforms and SDF Read or ReadAll


SDF Read and ReadAll are transforms too, right? The following will help clarify what the combination consists of: "...PeriodicSequence or PeriodicImpulse PTransforms and SDF Read or ReadAll PTransforms"

I might be missing something, but that's exactly the phrasing. "PTransforms" word is on next line.

I probably just missed the word. Looks good in staging.

soyrice · 2020-04-15T15:53:10Z

website/src/documentation/patterns/side-inputs.md

- a. MAX_TIMESTAMP can be replaced with some closer boundary if you want to stop generating elements at some point.
+1. Use the PeriodicImpulse or PeriodicSequence PTransform to generate an
+infinite sequence of elements at required processing time intervals and assign
+them to separate windows.

 1. Read data using Read operation triggered by arrival of PCollection element.


I see you marked the previous comment here as resolved but I don't see any changes to this line. Was that intended?

Was not intended. FIxed.

soyrice

The text LGTM after resolving the remaining comments, but I don't see the code sample on the staging server.

website/src/documentation/patterns/side-inputs.md

soyrice · 2020-04-15T17:06:06Z

website/src/documentation/patterns/side-inputs.md

+meaning that each window on the main input will be matched to a single
+version of side input data.
+
+Implementation of described approach can be narrowed down to:


We should rephrase as "To read side input data periodically:"

The goal is to summarize the previous paragraph, so that it's clear what these steps refer to without needing to read through the previous paragraph. The previous paragraph offers additional context, but if a reader skims the page, they should be able to quickly figure out what these steps are about.

soyrice · 2020-04-15T17:07:19Z

website/src/documentation/patterns/side-inputs.md

+ intervals
+ * Assign them to separate windows.
+1. Fetch data using SDF Read or ReadAll PTransform triggered by arrival of
+PCollection element.
 1. Apply side input.


Missing "the" -> "Apply the side input"

soyrice · 2020-04-15T17:10:02Z

The text LGTM after resolving the remaining comments, but I don't see the code sample on the staging server.

I think it's missing because it's not picking up the language selector.

Ardagan · 2020-04-15T17:16:01Z

Staging job failing together with website publish jobs :( So staging version has old code unfortunately.

soyrice · 2020-04-15T20:53:28Z

website/src/documentation/patterns/side-inputs.md

-
- a. MAX_TIMESTAMP can be replaced with some closer boundary if you want to stop generating elements at some point.
+You can read side input data periodically into distinct PCollection windows.
+Later, when you apply the side input to your main input, each main input


I think we should remove "Later" because this is a part of the overall workflow described in the previous sentence, rather than a secondary step.

So this should be: "When you apply the side input to your main input..."

soyrice · 2020-04-15T20:54:54Z

LGTM (modulo one last comment). Thanks again for opening this PR and doing an editorial review

Co-authored-by: Mikhail Gryzykhin <[email protected]>

probot-autolabeler bot added the website label Apr 14, 2020

Ardagan mentioned this pull request Apr 14, 2020

[BEAM-9650] Add PeriodicImpulse Transform and slowly changing side input documentation #11182

Merged

4 tasks

Ardagan force-pushed the SCPPyDocs branch from 4220160 to e935e77 Compare April 14, 2020 18:46

Ardagan commented Apr 14, 2020

View reviewed changes

Cleanup documentation

232357e

Ardagan force-pushed the SCPPyDocs branch from e935e77 to 232357e Compare April 14, 2020 18:50

soyrice reviewed Apr 14, 2020

View reviewed changes

Ardagan commented Apr 14, 2020

View reviewed changes

website/src/documentation/patterns/side-inputs.md Show resolved Hide resolved

Ardagan requested a review from pabloem April 14, 2020 21:32

Address comments

96814cb

soyrice reviewed Apr 15, 2020

View reviewed changes

Rephrase

c931ba9

soyrice reviewed Apr 15, 2020

View reviewed changes

one more rephrasing

a914d7c

soyrice reviewed Apr 15, 2020

View reviewed changes

One more comment

297d630

Ardagan merged commit 892a0a4 into apache:master Apr 17, 2020

Ardagan deleted the SCPPyDocs branch April 17, 2020 18:37

yirutang pushed a commit to yirutang/beam that referenced this pull request Jul 23, 2020

[BEAM-9650] Cleanup documentation on side inputs patterns (apache#11415)

b166fa3

Co-authored-by: Mikhail Gryzykhin <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-9650] Cleanup documentation on side inputs patterns #11415

[BEAM-9650] Cleanup documentation on side inputs patterns #11415

Ardagan commented Apr 14, 2020 •

edited

Loading

Ardagan commented Apr 14, 2020 •

edited

Loading

Ardagan Apr 14, 2020

soyrice left a comment

soyrice Apr 14, 2020

soyrice left a comment •

edited

Loading

soyrice Apr 15, 2020

Ardagan Apr 15, 2020

soyrice Apr 15, 2020

soyrice Apr 15, 2020

Ardagan Apr 15, 2020

soyrice Apr 15, 2020

soyrice Apr 15, 2020

Ardagan Apr 15, 2020

soyrice left a comment

soyrice Apr 15, 2020

soyrice Apr 15, 2020

soyrice commented Apr 15, 2020

Ardagan commented Apr 15, 2020

soyrice Apr 15, 2020

soyrice commented Apr 15, 2020

[BEAM-9650] Cleanup documentation on side inputs patterns #11415

[BEAM-9650] Cleanup documentation on side inputs patterns #11415

Conversation

Ardagan commented Apr 14, 2020 • edited Loading

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

Ardagan commented Apr 14, 2020 • edited Loading

Choose a reason for hiding this comment

soyrice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soyrice left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soyrice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soyrice commented Apr 15, 2020

Ardagan commented Apr 15, 2020

Choose a reason for hiding this comment

soyrice commented Apr 15, 2020

Ardagan commented Apr 14, 2020 •

edited

Loading

Ardagan commented Apr 14, 2020 •

edited

Loading

soyrice left a comment •

edited

Loading