Brijesh-Thakkar commented on issue #19713: URL: https://github.com/apache/beam/issues/19713#issuecomment-6526837253
Thanks, agreed that a tight check is risky. I looked closer at what would break: - Encoding with ProtoCoder<Message> works today (encode() never calls getParser()). Only decode fails. So rejecting Message.class in ProtoCoder.of() would break explicit encode-only use. I'd leave of() as is. - CoderRegistry inference (ProtoCoderProvider.coderFor) silently returns ProtoCoder<Message> for TypeDescriptor.of(Message.class) and for an erased T extends Message. That's the path in this issue, where it then fails on workers during shuffle. - I found no main code in Beam constructing ProtoCoder with raw Message. So I'd split it: 1. No behaviour change: in getParser(), replace the NoSuchMethodException with an actionable error (use a concrete generated class, DynamicProtoCoder, or setCoder()). This only touches a path that already always fails. Test asserts the new error and fails on master. 2. Optional: have ProtoCoderProvider.coderFor throw CannotProvideCoderException for raw Message, so it fails at construction. The only thing this breaks is a pipeline relying on an *inferred* raw-Message coder purely for encoding. A softer alternative would be logging a warning at inference instead of throwing. Would you prefer just (1), (1) + warning, or (1) + (2)? -- 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]
