lostluck commented on code in PR #32045:
URL: https://github.com/apache/beam/pull/32045#discussion_r1702190129
##########
sdks/go/pkg/beam/coder.go:
##########
@@ -51,7 +50,7 @@ type jsonCoder interface {
json.Unmarshaler
}
-var protoMessageType = reflect.TypeOf((*protov1.Message)(nil)).Elem()
Review Comment:
This is currently referencing
https://pkg.go.dev/github.com/golang/[email protected]/proto#Message
But if we want to avoid directly depending on the old package we can replace
it properly with
https://pkg.go.dev/google.golang.org/protobuf/protoadapt#MessageV1
WRT Go Protocol buffers the V1 package depends on the V2 package, so we can
maintain our desired behavior by importing the
"google.golang.org/protobuf/protoadapt" package and replacing the instances of
`protov1.Message` with `protoadapt.MessageV1`.
##########
sdks/go/pkg/beam/coder.go:
##########
@@ -276,8 +275,6 @@ func protoEnc(in T) ([]byte, error) {
switch it := in.(type) {
case protoreflect.ProtoMessage:
p = it
- case protov1.Message:
- p = protov1.MessageV2(it)
Review Comment:
This line and the identical one below can instead use:
https://pkg.go.dev/google.golang.org/protobuf/protoadapt#MessageV2Of
which has an identical implementation.
##########
sdks/go/pkg/beam/core/runtime/harness/statemgr.go:
##########
@@ -633,15 +633,16 @@ func (c *StateChannel) read(ctx context.Context) {
if !ok {
// This can happen if Send returns an error that write
handles, but
// the message was actually sent.
- log.Errorf(ctx, "StateChannel[%v].read: no consumer for
state response: %v", c.id, proto.MarshalTextString(msg))
+ bytes, _ := prototext.Marshal(msg)
Review Comment:
Either do what's already been in progress (using msg.String()), OR call
prototext.Format(msg) instead, which produces a human readable multiline output
of the proto.
##########
sdks/go/pkg/beam/core/runtime/graphx/translate.go:
##########
@@ -1209,13 +1209,13 @@ func (m *marshaller) addWindowingStrategy(w
*window.WindowingStrategy) (string,
}
func (m *marshaller) internWindowingStrategy(w *pipepb.WindowingStrategy)
string {
- key := proto.MarshalTextString(w)
- if id, exists := m.windowing2id[key]; exists {
+ key := w.String()
+ if id, exists := m.windowing2id[string(key)]; exists {
Review Comment:
Same here, we shouldn't need to cast the output of .String() to a string.
##########
sdks/go/pkg/beam/coder.go:
##########
@@ -51,7 +50,7 @@ type jsonCoder interface {
json.Unmarshaler
}
-var protoMessageType = reflect.TypeOf((*protov1.Message)(nil)).Elem()
+var protoMessageType = reflect.TypeOf((*protov2.Message)(nil)).Elem()
Review Comment:
This is a breaking change since the `protov2.Message` type isn't the same as
the `protov1.Message`, and we might end up losing support for user types that
happen to still be protov1.
##########
sdks/go/pkg/beam/core/runtime/graphx/coder.go:
##########
@@ -615,8 +615,8 @@ func (b *CoderMarshaller) internRowCoder(schema
*pipepb.Schema) string {
}
func (b *CoderMarshaller) internCoder(coder *pipepb.Coder) string {
- key := proto.MarshalTextString(coder)
- if id, exists := b.coder2id[key]; exists {
+ key := coder.String()
+ if id, exists := b.coder2id[string(key)]; exists {
Review Comment:
If key is already a string, we shouldn't need to cast it to a string. (here
and below)
##########
sdks/go/pkg/beam/coder.go:
##########
@@ -293,8 +290,6 @@ func protoDec(t reflect.Type, in []byte) (T, error) {
switch it := reflect.New(t.Elem()).Interface().(type) {
case protoreflect.ProtoMessage:
p = it
- case protov1.Message:
- p = protov1.MessageV2(it)
Review Comment:
And here as well. `protoadapt.MessageV2Of`
--
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]
