You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2021/08/30 16:31:31 UTC
[GitHub] [beam] lostluck commented on a change in pull request #15409: [BEAM-3304] Snippets for trigger in BPG
lostluck commented on a change in pull request #15409:
URL: https://github.com/apache/beam/pull/15409#discussion_r698613685
##########
File path: website/www/site/content/en/documentation/programming-guide.md
##########
@@ -4578,6 +4579,7 @@ firings:
{{< code_sample "sdks/python/apache_beam/examples/snippets/snippets_test.py" model_early_late_triggers >}}
{{< /highlight >}}
Review comment:
Since you've added snippets, I assume the intent is to include the appropriate block so they get displayed:
```
{{< highlight go >}}
{{< code_sample "sdks/go/examples/snippets/09triggers.go" model_early_late_triggers >}}
{{< /highlight >}}
```
And similar throughout.
You can see how things render using `./gradlew :website:serveWebsite ` to stand up a localhost version of the website. Note, for new snippet files you need to add at least one code_sample block for them to be known. I end up iterating between using the gradle command above, stopping that and then clearing the running docker containers `docker stop $(docker ps -q -f ancestor=beam-website)` to see fresh changes on the next run of the gradle command.
##########
File path: website/www/site/content/en/documentation/programming-guide.md
##########
@@ -4594,7 +4596,8 @@ modifying this behavior.
The `AfterProcessingTime` trigger operates on *processing time*. For example,
the <span class="language-java">`AfterProcessingTime.pastFirstElementInPane()`</span>
-<span class="language-py">`AfterProcessingTime`</span> trigger emits a window
+<span class="language-py">`AfterProcessingTime`</span>
+<span class="language-go">`Trigger{Kind: AfterProcessingTimeTrigger, Delay: 5000}`</span> trigger emits a window
Review comment:
can probably simplify this down to `window.AfterProcessingTimeTrigger` which matches how users will see it. We don't need to show the wrapping, that's what the code sample blocks are for.
##########
File path: website/www/site/content/en/documentation/programming-guide.md
##########
@@ -4607,14 +4610,16 @@ window.
Beam provides one data-driven trigger,
<span class="language-java">`AfterPane.elementCountAtLeast()`</span>
-<span class="language-py">`AfterCount`</span>. This trigger works on an element
+<span class="language-py">`AfterCount`</span>
+<span class="language-go">`Trigger{Kind: ElementCountTrigger, ElementCount: 2}`</span>. This trigger works on an element
count; it fires after the current pane has collected at least *N* elements. This
allows a window to emit early results (before all the data has accumulated),
which can be particularly useful if you are using a single global window.
It is important to note that if, for example, you specify
<span class="language-java">`.elementCountAtLeast(50)`</span>
-<span class="language-py">AfterCount(50)</span> and only 32 elements arrive,
+<span class="language-py">AfterCount(50)</span>
+<span class="language-go">`Trigger{Kind: ElementCountTrigger, ElementCount: 50}`</span> and only 32 elements arrive,
Review comment:
For contrast, this one indicates a specific number.
Note how verbose this is compared to Java and Python. Perhaps we should have helper functions like `window.TriggerAfterCount(50)` to make the user experience user easier? (it avoids users accidentally constructing bad triggers too)
##########
File path: website/www/site/content/en/documentation/programming-guide.md
##########
@@ -4679,6 +4695,13 @@ trigger. To set a window to discard fired panes, set `accumulation_mode` to
`DISCARDING`.
{{< /paragraph >}}
+{{< paragraph class="language-go" >}}
+To set a window to accumulate the panes that are produced when the trigger
+fires, set the `AccumulationMode{Mode:}` parameter to `AccumulationMode_ACCUMULATING` when you set the
Review comment:
The phrasing here is awkward. Don't try to present it as setting a parameter in these instances, demonstrate the code directly instead.
```
To set a window to accumulate the panes that are produced when the trigger
fires, use `beam.AccumulationMode{Mode: window.Accumulating}`.
```
etc.
##########
File path: website/www/site/content/en/documentation/programming-guide.md
##########
@@ -4679,6 +4695,13 @@ trigger. To set a window to discard fired panes, set `accumulation_mode` to
`DISCARDING`.
{{< /paragraph >}}
+{{< paragraph class="language-go" >}}
+To set a window to accumulate the panes that are produced when the trigger
+fires, set the `AccumulationMode{Mode:}` parameter to `AccumulationMode_ACCUMULATING` when you set the
Review comment:
It's not 100% clear to me whether we can make this a bit shorter for users by having convenience methods, other than using separate convenience method names: `beam.PanesAccumulate`, `beam.PanesDiscard`, (ignoring `beam.PanesRetract` for now since nothing supports it IIRC...)
##########
File path: website/www/site/content/en/documentation/programming-guide.md
##########
@@ -4623,7 +4628,7 @@ either when I receive 50 elements, or every 1 second".
### 9.4. Setting a trigger {#setting-a-trigger}
When you set a windowing function for a `PCollection` by using the
-<span class="language-java">`Window`</span><span class="language-py">`WindowInto`</span>
+<span class="language-java">`Window`</span><span class="language-py">`WindowInto`</span><span class="language-go">`WindowInto`</span>
Review comment:
In this case, to make it look more Go, clarify directly that it's `beam.WindowInto` since that's how users will always see it.
Note: Fun feature of CSS: If two blockers *would* have the same content, you can just put two classes in the span class argument. eg. `<span class="language-py language-go">.
##########
File path: website/www/site/content/en/documentation/programming-guide.md
##########
@@ -4643,6 +4648,13 @@ element in that window has been processed. The `accumulation_mode` parameter
sets the window's **accumulation mode**.
{{< /paragraph >}}
+{{< paragraph class="language-go" >}}
+You set the trigger(s) for a `PCollection` by passing in the `WindowTrigger` parameter
Review comment:
When directly referring to code constructs for Go, prefer the way it shows up in the code, including the usual package short name. eg. `beam.WindowTrigger`, `beam.WindowInto`, `beam.AccumulationMode` etc. (Since the PCollection type isn't being referred to in code, it doesn't get the package prefix.)
##########
File path: website/www/site/content/en/documentation/programming-guide.md
##########
@@ -4607,14 +4610,16 @@ window.
Beam provides one data-driven trigger,
<span class="language-java">`AfterPane.elementCountAtLeast()`</span>
-<span class="language-py">`AfterCount`</span>. This trigger works on an element
+<span class="language-py">`AfterCount`</span>
+<span class="language-go">`Trigger{Kind: ElementCountTrigger, ElementCount: 2}`</span>. This trigger works on an element
Review comment:
Similar to the above we can probably simplify this down to `window.ElementCountTrigger` which matches how users will see it. We don't need to show the wrapping, that's what the code sample blocks are for.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org