lostluck commented on a change in pull request #16980:
URL: https://github.com/apache/beam/pull/16980#discussion_r817930635
##########
File path: sdks/go/pkg/beam/core/runtime/exec/pardo.go
##########
@@ -220,6 +226,49 @@ func (n *ParDo) FinishBundle(_ context.Context) error {
return nil
}
+func (n *ParDo) FinalizeBundle(ctx context.Context) error {
+ failedIndices := []int{}
+ for idx, bfc := range n.bf.callbacks {
Review comment:
I still need to look at the new code but to answer your question:
WRT Compatibility, as a rule, we no longer make breaking changes to the main
beam package.
Attached to this are things the main beam package make use of or that users
use to build their own DoFns (eg the typex.Window interface, event time etc.)
We strongly avoid making changes to exported surfaces of our internal
packages, but can if it's the best way to do things.
We have a statement about compatibility in the [release
announcement](https://beam.apache.org/blog/go-sdk-release/) but this too is
woefully non-specific. It would be best if we didn't force this on our users,
but that's a larger set of refactorings.
In this specific case, extending the Unit interface would at worse also
require changes to the direct runner implementations. Technically the direct
runner is the only reason that interface is exposed at all (and frankly for
most of the exec package anyway...) (This is another reason I'm not a fan of
our direct runner).
As a rule for Go, adding methods to an interface is generally (but not
always) the Wrong Move. It's inherently a breaking change. Interfaces are best
when they're small. Especially if the cases are optional like here, it's often
better to define an additional interface and type assert to check if it
implements it. Now that alone wouldn't solve this plumbing necessary here.
In this case, it's a category error: One is treating the interface like a
Java abstract base class, where we can have a default implementation,
propagated to all subclasses automatically. However, outside of [Type
Embedding](https://eli.thegreenplace.net/2020/embedding-in-go-part-1-structs-in-structs/),
there's no such feature in Go, and type embedding itself needs careful design
to make use of it. In particular since it plays havok with trying to do things
with reflection on the embedded type...
--
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]
