lostluck commented on a change in pull request #14666:
URL: https://github.com/apache/beam/pull/14666#discussion_r623200102
##########
File path: sdks/go/examples/xlang/transforms.go
##########
@@ -17,12 +17,31 @@
package xlang
import (
+ "reflect"
+
"github.com/apache/beam/sdks/go/pkg/beam"
"github.com/apache/beam/sdks/go/pkg/beam/core/graph"
"github.com/apache/beam/sdks/go/pkg/beam/core/typex"
"github.com/apache/beam/sdks/go/pkg/beam/core/util/reflectx"
)
+func init() {
+ beam.RegisterType(reflect.TypeOf((*PrefixPayload)(nil)).Elem())
+}
+
+type PrefixPayload struct {
Review comment:
Looking at how the PrefixPayload is used, I'd recommend not exporting
the type, and avoiding over documenting the self explanatory type. It would
still need to be registered, (as you're doing) and have it's Data field
exported.
However, since this is part of the Xlang examples, it's reasonable to keep
it Exported and document what it is (a type to configure the Prefix transform),
how it's used (an internal type to be schema encoded so the xlang transform can
use interpret it), where that format comes from (github link to the Java
format/equivalent is fine) and that it's exported for example documentation
purposes only.
It's odd to me that the PrefixPayload on the receiving side is expecting a
schema with a "Data" field, but if that's correct, that's what it is. I'm glad
that it didn't need a field name override struct tag at least.
##########
File path: sdks/go/examples/xlang/transforms.go
##########
@@ -17,12 +17,31 @@
package xlang
import (
+ "reflect"
+
"github.com/apache/beam/sdks/go/pkg/beam"
"github.com/apache/beam/sdks/go/pkg/beam/core/graph"
"github.com/apache/beam/sdks/go/pkg/beam/core/typex"
"github.com/apache/beam/sdks/go/pkg/beam/core/util/reflectx"
)
+func init() {
+ beam.RegisterType(reflect.TypeOf((*PrefixPayload)(nil)).Elem())
+}
+
+type PrefixPayload struct {
+ Data string
+}
+
+func Prefix(s beam.Scope, prefix string, addr string, col beam.PCollection)
beam.PCollection {
Review comment:
Consider adding documentation for what the Prefix transform is, and what
it's testing/serving as an example of.
We should probably add similar documentation for all the transforms below as
well, but that's out of scope for this PR.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
