About Proto3 and Proto2:
1. *schema compatibility checking for proto3*
Proto3 canceled the required field, so in the current design, there is no
need to check the required field. We can get the syntax of proto in the
code, and skip the check of the required field if it is proto3. All other
checking rules should apply to proto3.
2. *schema compatibility checking between proto3 and proto2*
For `SchemaCompatibilityStrategy` as BACKWARD, FORWARD and FULL, if the two
proto file syntax is different I think we should just throw an
exception. Schema
incompatibility.
throw new ProtoBufCanReadCheckException("Protobuf syntax have been changed."
);Kiryl Valkovich <kiryl_valkovich@teal.tools> 于2023年2月20日周一 22:51写道： > [Edit] Sorry, it’s documented here: > https://pulsar.apache.org/docs/2.11.x/schema-understand/#schema-compatibility-check > > From: Kiryl Valkovich <kiryl_valkovich@teal.tools> > Date: Monday, February 20, 2023 at 3:48 PM > To: dev@pulsar.apache.org <dev@pulsar.apache.org> > Subject: Re: [DISCUSS] PIP-246: Improved PROTOBUF_NATIVE schema > compatibility checks without using avro-protobuf > Hm. I tend to think that for my Pulsar use case, it would be good to have > the ability to implement a custom schema compatibility checker. > > For example, buf.build (a popular Protobuf linter and build) offers the > following list of breaking rules. Half of them prefixed with > FIELD_SAME_XXLANG_PACKAGE_ aren’t relevant. > > And I would want to use a subset of them if we are checking a single > message descriptor. Most likely: > ENUM_VALUE_NO_DELETE > FIELD_NO_DELETE > FIELD_SAME_TYPE > ONEOF_NO_DELETE > FIELD_SAME_LABEL > RESERVED_ENUM_NO_DELETE > RESERVED_MESSAGE_NO_DELETE > > I found out that the Pulsar broker has the following configuration option: > schemaRegistryCompatibilityCheckers > [org.apache.pulsar.broker.service.schema.JsonSchemaCompatibilityCheck, > org.apache.pulsar.broker.service.schema.AvroSchemaCompatibilityCheck, > org.apache.pulsar.broker.service.schema.ProtobufNativeSchemaCompatibilityCheck] > > But I can’t find documentation for it. > > At first look, it seems that for my need it would be enough to have a such > custom configurable Protobuf message descriptor checker class that > implements SchemaCompatibilityCheck. > > > https://github.com/apache/pulsar/blob/82237d3684fe506bcb6426b3b23f413422e6e4fb/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/ProtobufNativeSchemaCompatibilityCheck.java#L31 > > [cid:image002.png@01D94541.991807E0] > > [Text Description automatically generated] > > From: SiNan Liu <liusinan1...@gmail.com> > Date: Monday, February 20, 2023 at 1:41 PM > To: dev@pulsar.apache.org <dev@pulsar.apache.org> > Subject: Re: [DISCUSS] PIP-246: Improved PROTOBUF_NATIVE schema > compatibility checks without using avro-protobuf > It seems that we should only warn the user about changes to the field type, > not define a compatibility check, or throw an exception. > *Just like this: * > *log.warn("proto.read.ProtobufSchema.uint64Field field type for uint64, was > changed into a uint32");* > > I will update this in the PIP issue Alternatives and discuss both designs > with other people. > > > Kiryl Valkovich <kiryl_valkovich@teal.tools> 于2023年2月20日周一 18:14写道： > > > 1. Sure, I didn’t mean that it's only about the required fields. > > 2. I found the page you are referring to. > > https://protobuf.dev/programming-guides/proto3/#updating > > > > QUOTE START > > If a number is parsed from the wire which doesn’t fit in the > corresponding > > type, you will get the same effect as if you had cast the number to that > > type in C++ (for example, if a 64-bit number is read as an int32, it will > > be truncated to 32 bits). > > QUOTE END > > > > It’s discussable. Maybe I’m just missing something. > > > > I personally wouldn’t rely on such compatibility guarantees in a real > > application. > > If my check amount (> int32 lol) would be truncated because someone > > changed the field type in a schema, I would be quite upset. > > > > From: SiNan Liu <liusinan1...@gmail.com> > > Date: Monday, February 20, 2023 at 2:30 AM > > To: dev@pulsar.apache.org <dev@pulsar.apache.org> > > Subject: Re: [DISCUSS] PIP-246: Improved PROTOBUF_NATIVE schema > > compatibility checks without using avro-protobuf > > Thank you for your suggestions and questions. > > 1. Your first question. It's not just a matter of the required field. > There > > should be many differences between proto3 and proto2. I will later test > the > > current code for compatibility checks in proto3, and also look at > > compatibility between different protobuf versions. > > 2. On your second question. This is the compatibility rule for field type > > changes I found on the official website. You mean that this rule should > not > > pass the compatibility check. Or should an exception be thrown whenever > the > > field type changes? > > > > > > Kiryl Valkovich <kiryl_valkovich@teal.tools> 于 2023年2月20日周一 上午12:31写道： > > > > > First, thank you for looking into it! > > > > > > There is a thing caught my eye: > > > > > > > The writtenSchema cannot add required fields, … > > > > > > As far as I know, the required fields were removed in Protocol Buffers > > v3. > > > > > > I see proto3 usage in existing schema registry tests: > > > > > > > > > https://github.com/apache/pulsar/blob/6704f12104219611164aa2bb5bbdfc929613f1bf/pulsar-broker/src/test/proto/ProtobufSchemaTest.proto#L19 > > > > > > > > > > > > https://github.com/apache/pulsar/blob/1ea4ad814f5f30b8c371db2a86626cd568ace553/pulsar-sql/presto-pulsar/src/test/java/org/apache/pulsar/sql/presto/decoder/protobufnative/TestMsg.proto#L19 > > > > > > I see proto2 usage in the proposed changes: > > > > > > > > > > > > https://github.com/apache/pulsar/pull/19566/files#diff-f2f7463a3df6dc6366f3ee3a416707e655f0d5b8d2ae623a3f05a209c1d6ec88R19 > > > > > > > > > > > > https://github.com/apache/pulsar/pull/19566/files#diff-f2f7463a3df6dc6366f3ee3a416707e655f0d5b8d2ae623a3f05a209c1d6ec88R19 > > > > > > Also, I’m not sure about this: > > > > > > > int32, uint32, int64, uint64, and bool are all compatible – this > means > > > you can change a field from one of these types to another without > > breaking > > > forwards- or backwards-compatibility. > > > > > > It leads to JS/PHP-like implicit conversions if I understand it right. > > > > > > From: SiNan Liu <liusinan1...@gmail.com> > > > Date: Sunday, February 19, 2023 at 1:59 PM > > > To: dev@pulsar.apache.org <dev@pulsar.apache.org> > > > Subject: [DISCUSS] PIP-246: Improved PROTOBUF_NATIVE schema > compatibility > > > checks without using avro-protobuf > > > Hi all, > > > > > > I made a PIP to discuss: https://github.com/apache/pulsar/issues/19565 > . > > > > > > We can talk about the current design here. Especially for the field > type > > > change check rules, please give your valuable advice. > > > > > > Thanks, > > > Sinan > > > > > >
