lostluck commented on a change in pull request #15239:
URL: https://github.com/apache/beam/pull/15239#discussion_r681123176
##########
File path: sdks/go/pkg/beam/core/graph/edge.go
##########
@@ -527,8 +527,8 @@ func NewImpulse(g *Graph, s *Scope, value []byte)
*MultiEdge {
}
// NewWindowInto inserts a new WindowInto edge into the graph.
-func NewWindowInto(g *Graph, s *Scope, wfn *window.Fn, in *Node) *MultiEdge {
- n := g.NewNode(in.Type(), &window.WindowingStrategy{Fn: wfn},
in.Bounded())
+func NewWindowInto(g *Graph, s *Scope, wfn *window.Fn, tr window.TriggerType,
in *Node) *MultiEdge {
Review comment:
NewWindowInto should just change to take in a
`*window.WindowingStrategy` instead of adding more and more parameters to it.
There are other modes to handle in the future and by populating the windowing
strategy in the Beam package before it gets here, will save time and effort
when adding features later.
##########
File path: sdks/go/pkg/beam/windowing.go
##########
@@ -21,21 +21,47 @@ import (
"github.com/apache/beam/sdks/go/pkg/beam/internal/errors"
)
+type WindowIntoOption interface {
+ private()
Review comment:
"private" isn't a good name for this blocking function, because
beam.Option is already using it. You can test this out yourself by seeing that
you can pass `WindowIntoOption`s as an Option into beam.ParDo.
A better unexported method name would be `windowIntoOption()` to avoid this
conflation.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
