On Tue, Mar 7, 2023 at 1:01 PM SiNan Liu <liusinan1...@gmail.com> wrote:
> > > > Ok. > > First, the name is confusing. Flags are normally true/false, in your case > > it's a string, so the name should be a Configuration property. > > Second, I agree - you're basically saying we must allow users to keep > > existing implementation, or switch to new implementation. Just using a > > boolean feature flag might suffice now, but how will we improve the rules > > without breaking existing users. So we choose to encode the new code as a > > new class and have the user select it via config, by specifying the class > > name. > > 3rd, now that I understand that, you need to revise your explanation. > It's > > completely unclear from the current explanation. > > > The description here should not be flag, but configuration. I will modify > it. > > Of course you shouldn't make a gigantic class having all validation > > methods. I'm fine with having > > `ProtobufNativeBackwardComptabilityValidation` class and each type of > > compatibility strategy can have its own class, but just have it simple: > > each strategy is mapped to one simple validation method. I don't see any > > need for the added layers you have there of the builders, etc - current > > Avro design is too complicated, so let's not copy it. > > I understand > > As I wrote before, using `writtenSchema` and `readSchema` is confusing. > > Example: You write: ..... > > This I can read and understand. > > > 1. All the things you said. Explain the checking rules for each > schema-compatible-check-strategy separately, indicating that you do not > understand the current design. I've explained this many times. > In `Implementation` 1: > ``` > Schema compatibility checking can be understood as producer (write) > consumer (read). If a producer uses one schema to write data to the topic > and a consumer can read it, then the two schemas are compatible. A > "writtenSchema" canRead by a "readSchema". > ``` > Whether two schemas are compatible is whether they can be ****read**** by > each other. If they cannot be read, they are incompatible!!! > I perfectly understand that, but as I said before: 1. a new schema can be introduced by a producer - which from now on will use that new schema to write messages --> written schema. Or, a new schema can be introduced by a consumer - the consumer will register this new schema and use it to read messages written by the previous schema. Now, what do you think we should call the new schema in this case? In the first case I described: new schema is written shema, existing schema is the read schema. In the second case I described: the new schema is the read schema, and the existing schema is the written schema. So while I understand what you're saying, you need to realize that there are two ways to register a new schema for the schema registry, hence we shouldn't call the new schema writtenSchema because as I explained, in the case of a new consumer registering it, it's wrong. Regarding saying I don't understand the current design - you are correct. I'm still trying and spending HUGE amounts of time to do so. I have vast experience in distributed systems and you can ask the PMC members here such as Matteo (PMC chair) or Penghui what's the level of my knowledge. Believe me when I say that if *I* can't understand your design, a big chunk of Pulsar users and developers won't. So my time spent here is done to verify not only you and 2 other people can read and understand it. That's easy. It's that *the rest of us* in the community both user and dev will understand it. It ain't the student's fault my friend. > > 2. Instead of discussing what the validation rules are for each > schema-compatible-check-strategy, find out what the schema compatibility > checking behavior looks like and build the Validator! All it takes is three > parameters! > In the Implementation of the 2, we are through > ProtobufNativeSchemaValidatorBuilder build different according to different > parameters of the Validator. > (1) ProtobufNativeSchemaValidationStrategy: According to the description of > `schema-compatibility-check-strategy` on pulsar official website, And the > description of "writtenSchema" (schema used by the Producer) and > "readSchema" (schema used by the Consumer) in Implementation 1. The > schema-compatibility-check-strategy mapping to > `ProtobufNativeSchemaValidationStrategy` of three strategies. > (2) isOnlyValidateLatest: This parameter is explained in `Implementation 2 > (2)`. This also maps to what kind of checking behavior TRANSITIVE is > .(whether to check only the last schema that already exists) > (3) validatorClassName: This is the validator implementation class, which > by default checks only the name of the root message. > > 3. Once the Validator has been created, don't worry about every > schema-compatibility-check-strategy at this point! Only need to consider > three `ProtobufNativeSchemaValidationStrategy` how to achieve! > ``` > So the existsSchema here is V1 and V2 schema, and the newSchema is schema > V3. We just need to change the arguments that are passed in to canRead > check. > ``` > Here we map existsSchema and newSchema directly to writtenSchema and > readSchema. > (V1,V2 is existsSchema. V3 is newSchema) > > 4. Finally, the implementation of `canRead`. And don't say anything about > what the rules for each schema-compatibility-check-strategy are! The > inspection rules are the same, as described on protobuf website. > `According to the protobuf official website > https://protobuf.dev/programming-guides/proto/#updating for compatibility. > Here are the basic compatibility rules we've defined:` > > 5. > > > What you linked (https://protobuf.dev/programming-guides/proto/#updating > ) > > are a bunch of guidelines and rules. > > It's not what you are actually going to validate. > > > *That's the most important thing. canRead is a rule defined on the official > website. It only checks whether two protobufs are compatible.* > *schema-compatibility-check-strategy not considered! This is described in > Implementation 2 and on the pulsar official website.* > > 6. Why implement each schema-compatibility-check-strategy separately? Isn't > it good to find out the commonality of the compatibility checking behavior > of multiple policies? > Yes, it is good to find the commonalities. But it is also good to write code people can understand. First, thanks for the explanation - now I understand what you wanted to achieve. The problem is I still think it's complicated :) You basically say: I want to encode something like: * Should I compare only the last version or all? * I will input the read schema and the write schema - sometimes they will be a list, sometimes they will be one depending on which direction you're looking at. and then run the validation. So you encode all of that into a validator as its "arguments" and then you just call validate. Seems fine, except that it hides the logic and makes it hard for newcomers to understand. Here is what I suggest - after this snippet you will find a detailed explanation of it. interface ProtobufCompatabilityValidator { readerSchemaCanReadWriterSchema(readerSchema, writerSchema) } BACKWARD: newSchema canRead existingSchema ==> protobufCompatabilityValidator.readerSchemaCanReadWriterSchema( readerSchema = newSchema writerSchema = existingSchema ) BACKWARD_TRANSITIVE: newSchema canRead all existingSchema ==> for (existingSchema : existingSchema) { protobufCompatabilityValidator.readerSchemaCanReadWriterSchema( readerSchema = newSchema writerSchema = existingSchema ) } FORWARD: existingSchema canRead newSchema ==> protobufCompatabilityValidator.readerSchemaCanReadWriterSchema( readerSchema = existingSchema writerSchema = newSchema ) FORWARD_TRANSITIVE: all existingSchema canRead newSchema ==> for (existingSchema : existingSchema) { protobufCompatabilityValidator.readerSchemaCanReadWriterSchema( readerSchema = existingSchema writerSchema = newSchema ) } FULL: newSchema canRead existingSchema existingSchema canRead newSchema ==> backward() forward() FULL_TRANSITIVE: all existingSchema canRead newSchema newSchema canRead all existingSchema ==> backwardTransitive() forwardTransitive() ProtobufCompatabilityValidator is the interface you wanted to expose as a configuration option, so people can choose the different implementations for it. I tried giving it a reasonable name and argument names so it's easy to figure out what it does. This is your encapsulation. Before that, I add a small explanation: When you define a topic, you configure a *single* compatibility check strategy. This strategy dictates what to check exactly when a new schema is registered. Sometimes it is the consumer who registers the new schema, and sometimes it is the producer registering the new schema. One thing is always correct: Messages are written using a certain schema (version), and later read using a different schema (version) The checks those strategies dictate can be summarized as follows: - BACKWARD strategy - newSchema can read existingSchema - BACKWARD_TRANSITIVE strategy - newSchema can read all existingSchema - FORWARD - existingSchema can read newSchema - FORWARD_TRANSITIVE - all existingSchema can read newSchema - FULL - newSchema can read existingSchema - existingSchema can read newSchema - FULL_TRANSITIVE - newSchema can read all existingSchema - all existingSchema can read newSchema then, I'll explain. We introduce a new interface called `ProtobufComptabilityValidator`, which will have two implementations. This object goal is to implement all the validation rules needed to verify whether messages written using a certain schema can be read by a different schema. The interface will look like this: interface ProtobufCompatabilityValidator { readerSchemaCanReadWriterSchema(readerSchema, writerSchema) } One implementation will encode the current logic [EXPAND], and the new implementation we will add is the one encoding a list of validation rules we will specify below based on the best practices in the protobuf site (place link here). Once we need to validate the new schema, we will act according to the strategy configured by the user for this topic: BACKWARD: protobufCompatabilityValidator.readerSchemaCanReadWriterSchema( readerSchema = newSchema writerSchema = existingSchema ) BACKWARD_TRANSITIVE: for (existingSchema : existingSchema) { protobufCompatabilityValidator.readerSchemaCanReadWriterSchema( readerSchema = newSchema writerSchema = existingSchema ) } FORWARD: protobufCompatabilityValidator.readerSchemaCanReadWriterSchema( readerSchema = existingSchema writerSchema = newSchema ) FORWARD_TRANSITIVE: for (existingSchema : existingSchema) { protobufCompatabilityValidator.readerSchemaCanReadWriterSchema( readerSchema = existingSchema writerSchema = newSchema ) } FULL: // what's written above backward() forward() FULL_TRANSITIVE: // what's written above backwardTransitive() forwardTransitive() Essentially a switch case. Easy, concise, straight forward. I find it hard to believe someone will have a hard time understanding the design with this explanation. Now, based on everything I wrote above, you must be *explicit* and write a detailed explanation of the rules you plan to implement in your implementation of `ProtobufComptabilityValidator`, based on those best practices. Thanks! Asaf > > > Thanks, > sinan > > > > > Asaf Mesika <asaf.mes...@gmail.com> 于2023年3月7日周二 17:45写道： > > > On Tue, Mar 7, 2023 at 6:51 AM SiNan Liu <liusinan1...@gmail.com> wrote: > > > > > Thanks for the advice, Asaf. > > > > > > 1. > > > > > > > For now, there is no official or third-party solution for ProtoBuf > > > > compatibility. If in the future have better solutions of a third > party > > or > > > > the official, we develop new ProtobufNativeSchemaValidator and use, > so > > > add > > > > a flag. > > > > > > Flag defaults not set, where the schema Compatibility Checking Rule > > checks > > > only the name of the root message. If you want to use the current PIP > > > implementation, Can be set to > > > > > > > > > `org.apache.pulsar.broker.service.schema.validator.ProtobufNativeSchemaBreakValidatorImpl`. > > > This explains what the flag does, and I'm not going to delete it. The > > user > > > can choose whether to use the previous implementation (just check that > > the > > > root message name is the same), but this may not be enough, so you can > > > choose the current PIP implementation. If there is a better third party > > or > > > official solution in the future, it can be well developed and replaced. > > It > > > is necessary to add the flag to the PIP and keep the implementation > > > extensible. > > > > > > > Ok. > > First, the name is confusing. Flags are normally true/false, in your case > > it's a string, so the name should be a Configuration property. > > Second, I agree - you're basically saying we must allow users to keep > > existing implementation, or switch to new implementation. Just using a > > boolean feature flag might suffice now, but how will we improve the rules > > without breaking existing users. So we choose to encode the new code as a > > new class and have the user select it via config, by specifying the class > > name. > > 3rd, now that I understand that, you need to revise your explanation. > It's > > completely unclear from the current explanation. > > > > > > > > > > > > > > > > 2. > > > > > > > Why not have a simple function for validation for each switch case > > above? > > > > Why do we need strategy and builder, and all this complexity? > > > > > > I don't see how it's complicated. It's easy to understand and it's not > > > redundant. The only function of the validator is to check whether two > > > Protobufs are compatible. The builder builds checkers based on > different > > > compatibility checking strategies. If all the implementation is done in > > the > > > validator, it will be messy and there will be a lot of duplication. And > > if > > > a new validator is extended later, it won't extend well. So discarding > > > encapsulation is more complex and loses scalability. I won't change > this > > > design. > > > > > > > Of course you shouldn't make a gigantic class having all validation > > methods. I'm fine with having > > `ProtobufNativeBackwardComptabilityValidation` class and each type of > > compatibility strategy can have its own class, but just have it simple: > > each strategy is mapped to one simple validation method. I don't see any > > need for the added layers you have there of the builders, etc - current > > Avro design is too complicated, so let's not copy it. > > > > > > > > > > > > 3. *Here are the basic compatibility rules we've defined:* > > > https://protobuf.dev/programming-guides/proto/#updating > > > According to the rules formulated by the official website, not > customized > > > by myself. > > > > > > > I understand > > > > As I wrote before, using `writtenSchema` and `readSchema` is confusing. > > > > Example: You write: > > > > > (1) Create： > > > > > > - The writtenSchema cannot add required fields, but optional or > > > duplicate fields can be added (The field number must be new). > > > > > > You said writtenSchema is the schema of the producer. > > What do you mean by create? Do you mean the producer is publishing the > > first schema ever to the registry? > > If so, what if you explain: > > > > newSchema - a new schema registered either by a producer or a consumer. > > currentSchema - the current latest schema in the schema registry > > > > BACKWARD: > > > > - new fields added in newSchema compared with currentSchema > > - New fields are defined as fields with field numbers that don't > > exist in the existing schema > > - The new fields must be optional and not required. > > - Modifying fields in the newSchema which already exists (update) in > the > > currentSchema > > - Modifying is defined as changing field details for the same field > > number. > > - Changing the name is permitted. > > - The type can be changed only if the type is compatible. The > allowed > > changes are .... > > - Removing fields in the newSchema which exists in the currentSchema > > - Removing a field is defined as removing the usage of the field > > number, used by a field in the currentSchema, so it won't exist in > > the > > schema. > > - ... > > > > > > This I can read and understand. > > > > You wrote > > > > > > > > - The writtenSchema do not change the field number of any field in > > > 'readSchema' (the field name is the same, but the field number is > > > different). > > > > > > So if I have > > > > oldSchema > > 1 customerName String > > 2 customerAge integer > > > > newSchema > > reserved 1 > > 2 customerAge integer > > 3 customerName string > > > > This is perfectly valid. > > > > I actually think we should force that - when you remove a field (field > > number removed), but add the same definition using a different field > > number, we should force you to declare reserved. > > > > > > What you linked (https://protobuf.dev/programming-guides/proto/#updating > ) > > are a bunch of guidelines and rules. > > It's not what you are actually going to validate. > > > > So how can I review your validation rules, if you don't write them > > explicitly? > > 1. By compatibility check strategy? > > 2. use correct naming, and not read and write schema, as the consumer can > > publish new schema and producer can publish new schema as well. Use > current > > and new to differentiate what you are comparing. > > 3. Write the rules *explicitly*. All rules you will use. For example, > types > > you allow changing (you wrote that under alternatives). > > > > > > > > > > > > 4. Other parts have been updated with explanations. > > > > > > > Sinan, listen - I spend *a lot* of time reviewing this PIP. Many rounds. > > You write "all parts updated", yet I go to the PIP and I see my first > > comment from the previous email was not implemented, nor got any reply > > here. > > > > Can you please review all of my previous comments and reply to each one > of > > them? > > > > > > Please bear in mind - all the work we're doing here in the review is: > > 1. Help future engineers understand the code better > > 2. Help future users understand this feature better > > 3. Make Pulsar look good. When we have designs which are super hard to > read > > and understand, it reflects badly on the community as a whole. > > 4. Once the doc is crystal clear, you can actually spend the time to find > > the faults in it. > > > > Thanks! > > > > Asaf > > > > > > > > > > > > > > > Thanks, > > > sinan > > > > > > > > > > > > > > > > > > > > > > > > 丛搏 <bog...@apache.org> 于2023年3月7日周二 11:53写道： > > > > > > > I think we have two ways to do that. > > > > > > > > First way: We need to advance the improvement of java in protobuf. > Ask > > > > if they have plans to improve. > > > > > > > > Second way: the new PROTOBUF_NATIVE `SchemaCompatibilityCheck` should > > > > be implemented as a plugin, don't change any existing plugin logic > > > > (it's simple and already used). I don't recommend adding flags for > > > > rollback, it adds configuration and makes little sense. > > > > > > > > Thanks, > > > > Bo > > > > > > > > Asaf Mesika <asaf.mes...@gmail.com> 于2023年3月6日周一 23:00写道： > > > > > > > > > > > > > > Can you convert the code block which is actually a quote in the > > > > > beginning of the PIP to something which doesn't require to scroll > > > > > horizontally so much? > > > > > Use > > > > > > > > > > > > > > > https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax#quoting-text > > > > > > > > > > Let's improve the clarity of what you wrote: > > > > > > > > > > "the PROTOBUF uses avro struct to store." > > > > > --> > > > > > When Schema type PROTOBUF is used, Pulsar Client assumes the object > > > given > > > > > to it as message data is an auto-generated POJO containing the > > > > annotations > > > > > encoding the schema. The client is using a converter, which > converts > > a > > > > > Protobuf schema descriptor into an Avro schema and sends that as > the > > > > Schema > > > > > of the producer/consumer. > > > > > > > > > > "On the broker side, protobuf and avro both use SchemaData > converted > > to > > > > > org.apache.avro.Schema." > > > > > --> > > > > > Since the schema is an Avro schema, the implementation of > > compatibility > > > > > check on the broker side is to simply re-use the compatibility > check > > of > > > > the > > > > > AVRO schema type. > > > > > > > > > > "ProtobufSchema is different from ProtobufNativeSchema in schema > > > > > compatibility check it uses avro-protobuf. > > > > > > > > > > > > > > > https://central.sonatype.com/artifact/org.apache.avro/avro-protobuf/1.11.1/overview > > > > > But the current implementation of ProtobufNative schema > compatibility > > > > > check only > > > > > checked if the root message name is changed." > > > > > > > > > > --> > > > > > PROTOBUF_NATIVE schema type is different. > > > > > The client is actually using Protobuf Descriptor as the schema, as > > > > opposed > > > > > to Avro schema of PROTOBUF schema type. In the broker, the > > > > PROTOBUF_NATIVE > > > > > compatibility check actually hasn't implemented any rule, besides > > one: > > > > > checking if the root message name has changed. > > > > > > > > > > > > > > > > > > > > > 1. For now, there is no official or third-party solution for > > > > ProtoBuf > > > > > > compatibility. If in the future have better solutions of a > third > > > > party or > > > > > > the official, we develop new ProtobufNativeSchemaValidator and > > > use, > > > > so > > > > > > add a flag. > > > > > > > > > > > > Who do you need to make that configurable? Once you found a third > > > > party, > > > > > just switch to it? Who knows, maybe you never will. Introduce it > when > > > you > > > > > find it, not now. > > > > > > > > > > > > > > > We improve in ProtobufNativeSchemaCompatibilityCheck BACKWARD, > > FORWARD > > > > > > these strategies. As with the AVRO implementation, protobuf > > > > compatibility > > > > > > checking need implementing the canRead method. *This will check > > that > > > > > > the writtenschema can be read by readSchema.* > > > > > > > > > > > > > > > I completely disagree. > > > > > Avro implementation is confusing for our use case. Don't copy that. > > > > > > > > > > You have > > > > > > > > > > public void checkCompatible(SchemaData from, SchemaData to, > > > > > SchemaCompatibilityStrategy strategy) > > > > > throws IncompatibleSchemaException { > > > > > Descriptor fromDescriptor = > > > > > ProtobufNativeSchemaUtils.deserialize(from.getData()); > > > > > Descriptor toDescriptor = > > > > > ProtobufNativeSchemaUtils.deserialize(to.getData()); > > > > > switch (strategy) { > > > > > case BACKWARD_TRANSITIVE: > > > > > case BACKWARD: > > > > > case FORWARD_TRANSITIVE: > > > > > case FORWARD: > > > > > case FULL_TRANSITIVE: > > > > > case FULL: > > > > > checkRootMessageChange(fromDescriptor, toDescriptor, > > > > strategy); > > > > > return; > > > > > case ALWAYS_COMPATIBLE: > > > > > return; > > > > > default: > > > > > throw new IncompatibleSchemaException("Unknown > > > > > SchemaCompatibilityStrategy."); > > > > > } > > > > > } > > > > > > > > > > I would rename : > > > > > from --> currentSchema > > > > > to --> newSchema > > > > > > > > > > Use that switch case and have a method for each like: > > > > > validateBackwardsCompatibility(currentSchema, newSchema) > > > > > > > > > > I dislike canRead and usage of writtenSchema, since you have two > > > > completely > > > > > different use cases: from the producing side and the consumer side. > > > > > > > > > > schemaValidatorBuilder > > > > > > > > > > > > I dislike this proposal. IMO Avro implementation is way too > > > > complicated. > > > > > Why not have a simple function for validation for each switch case > > > above? > > > > > Why do we need strategy and builder, and all this complexity? > > > > > > > > > > > > > > > *Here are the basic compatibility rules we've defined:* > > > > > > > > > > > > > > > IMO it's impossible to read the validation rules as you described > > them. > > > > > I wrote how they should be structured numerous times above. > > > > > I can't validate them. > > > > > > > > > > > > > > > IMO, the current design is very hard to read. > > > > > Please try to avoid jumping into code sections. > > > > > Write a high level design section, in which you describe in words > > what > > > > you > > > > > plan to do. > > > > > Write the validation rules in the structure that is easy to > > understand: > > > > > rules per each compatibility check, and use proper words (current > > > schema, > > > > > new schema), since new schema can be once used for read and once > used > > > for > > > > > write. > > > > > > > > > > In its current form it takes too much time to understand the > design, > > > and > > > > it > > > > > shouldn't be the case. > > > > > > > > > > Thanks, > > > > > > > > > > Asaf > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Sun, Mar 5, 2023 at 3:58 PM SiNan Liu <liusinan1...@gmail.com> > > > wrote: > > > > > > > > > > > Hi! I updated the explanation of some things in the PIP issue. > And > > > also > > > > > > added a new “flag” in the conf is used as the different > > > > > > ProtobufNativeSchemaValidator implementation, also set > > > > > > ProtobufNativeSchemaValidator default only check whether the name > > of > > > > the > > > > > > root message is the same. > > > > > > > > > > > > > > > > > > Thanks, > > > > > > sinan > > > > > > > > > > > > > > > > > > Asaf Mesika <asaf.mes...@gmail.com> 于2023年3月5日周日 20:21写道： > > > > > > > > > > > > > On Wed, Mar 1, 2023 at 4:33 PM SiNan Liu < > liusinan1...@gmail.com > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > Can you please explain how a Protobuf Schema descriptor can > > be > > > > > > > validated > > > > > > > > > for backward compatibility check using Avro based > > compatibility > > > > > > rules? > > > > > > > > > Doesn't it expect the schema to be Avro, but it is > actually a > > > > > > Protobuf > > > > > > > > > descriptor? > > > > > > > > > Is there some translation happening? > > > > > > > > > > > > > > > > > > > > > > > > 1. *You can take a quick look at the previous design, the > > > PROTOBUF > > > > uses > > > > > > > > avro struct to store.* > > > > > > > > https://github.com/apache/pulsar/pull/1954 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/apache/pulsar/blob/579f22c8449be287ee1209a477aeaad346495289/pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/ProtobufSchema.java#L59-L61 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/apache/pulsar/blob/579f22c8449be287ee1209a477aeaad346495289/pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/ProtobufSchema.java#L110-L115 > > > > > > > > > > > > > > > > > > > > > Ok. So to summarize your code (easier to write it than send > > links): > > > > > > > * Pulsar Client, when used with Protobuf Schema, actually > > converts > > > > the > > > > > > > Protobuf descriptor into an Avro Schema (using code found > inside > > > Avro > > > > > > > library) and saves that Avro schema as the schema. It's not > > saving > > > > the > > > > > > > protobuf descriptor at all. Very confusing I have to add - > never > > > > expected > > > > > > > that. > > > > > > > This explains why In the ProtobufSchemaCompatibilityCheck they > > just > > > > > > extend > > > > > > > the Avro without doing any translation. > > > > > > > > > > > > > > Thanks for that. > > > > > > > > > > > > > > Now thatI finally understand this, I can say that: you *must* > > > explain > > > > > > that > > > > > > > in the motivation part in your PIP. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 2. *On the broker side, protobuf and avro both use > `SchemaData` > > > > > > converted > > > > > > > > to `org.apache.avro.Schema`.* > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/apache/pulsar/blob/579f22c8449be287ee1209a477aeaad346495289/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java#L1280-L1293 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/apache/pulsar/blob/579f22c8449be287ee1209a477aeaad346495289/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/ProtobufSchemaCompatibilityCheck.java#L26-L31 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/apache/pulsar/blob/579f22c8449be287ee1209a477aeaad346495289/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/AvroSchemaBasedCompatibilityCheck.java#L47-L70 > > > > > > > > > > > > > > > > > > > > > Actually those links don't really help. > > > > > > > The main link that helps is: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/apache/pulsar/blob/ec102fb024a6ea2b195826778300f20e330dff06/pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/ProtobufSchema.java#L102-L122 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I'm sorry - I don't understand. > > > > > > > > > I understand the different compatibility check strategies. > > > > > > > > > If you just spell them out here, then as you say, just > > > translate > > > > the > > > > > > > > > Protobuf Descriptor into an Avro schema and run the Avro > > > > > > > > > compatibility validation, no? > > > > > > > > > I believe the answer is no, since you may want to verify > > > > different > > > > > > > things > > > > > > > > > when it comes to Protobuf, which are different then Avro. > > > > > > > > > > > > > > > > > > > > > > > > 1. > > > > > > > > *ProtobufSchema is different from ProtobufNativeSchema in > that > > it > > > > uses > > > > > > > > avro-protobuf.* > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://central.sonatype.com/artifact/org.apache.avro/avro-protobuf/1.11.1/overview > > > > > > > > *ProtobufNativeSchema needs a native compatibility check, but > > > > there is > > > > > > no > > > > > > > > official or third party implementation. So this PIP does not > > use > > > > > > > > avro-protobuf for protobuf compatibility checking.* > > > > > > > > > > > > > > > > 2. *By the way, this is implemented in much the same way that > > > > Apache > > > > > > avro > > > > > > > > does compatibility checking.* > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/apache/avro/blob/master/lang/java/avro/src/main/java/org/apache/avro/SchemaValidatorBuilder.java > > > > > > > > `canReadStrategy`，`canBeReadStrategy`，`mutualReadStrategy` > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/apache/avro/blob/master/lang/java/avro/src/main/java/org/apache/avro/ValidateCanRead.java > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/apache/avro/blob/master/lang/java/avro/src/main/java/org/apache/avro/ValidateCanBeRead.java > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/apache/avro/blob/master/lang/java/avro/src/main/java/org/apache/avro/ValidateMutualRead.java > > > > > > > > *In `ValidateMutualRead.java`, the arguments of `canRead()` > are > > > > > > > > writtenSchema and readSchema. We only need to change the > order > > of > > > > > > > arguments > > > > > > > > we pass to `canRead()`.* > > > > > > > > ```java > > > > > > > > private void validateWithStrategy(Descriptors.Descriptor > > > > toValidate, > > > > > > > > Descriptors.Descriptor fromDescriptor) throws > > > > > > > ProtoBufCanReadCheckException > > > > > > > > { > > > > > > > > switch (strategy) { > > > > > > > > case CanReadExistingStrategy -> canRead(fromDescriptor, > > > > toValidate); > > > > > > > > case CanBeReadByExistingStrategy -> canRead(toValidate, > > > > > > fromDescriptor); > > > > > > > > case CanBeReadMutualStrategy -> { > > > > > > > > canRead(toValidate, fromDescriptor); > > > > > > > > canRead(fromDescriptor, toValidate); > > > > > > > > } > > > > > > > > } > > > > > > > > } > > > > > > > > > > > > > > > > private void canRead(Descriptors.Descriptor writtenSchema, > > > > > > > > Descriptors.Descriptor readSchema) throws > > > > > > ProtoBufCanReadCheckException { > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > ProtobufNativeSchemaBreakCheckUtils.checkSchemaCompatibility(writtenSchema, > > > > > > > > readSchema); > > > > > > > > } > > > > > > > > ``` > > > > > > > > > > > > > > > > > > > > > > > I get that you want to take inspiration from the existing Avro > > > Schema > > > > > > > compatibility check, to do your code design. > > > > > > > I also understand you *won't* use any existing avro code for > > that. > > > > > > > I also understand, you have to write the validation check on > your > > > > own, > > > > > > > since there is no 3rd party to explain that. > > > > > > > > > > > > > > The only thing I can't understand are the actual rules as I > wrote > > > > before, > > > > > > > since they are written confusingly. > > > > > > > So, I repeat what I asked before: > > > > > > > > > > > > > > I think you should structure the validation rules differently: > > > > > > > > > > > > > > * Backward checks > > > > > > > ** List down rules, where use newSchema (the schema used by > > > producer > > > > or > > > > > > > consumer) and existingSchema (last schema used) > > > > > > > * Forward > > > > > > > ** List down rules, where use newSchema (the schema used by > > > producer > > > > or > > > > > > > consumer) and existingSchema (last schema used) > > > > > > > > > > > > > > Once that's accomplished I will be able to understand the > > different > > > > > > > validation rules for each compatibility check. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > sinan > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Asaf Mesika <asaf.mes...@gmail.com> 于2023年3月1日周三 21:19写道： > > > > > > > > > > > > > > > > > > On Mon, Feb 27, 2023 at 3:47 PM SiNan Liu < > > > > liusinan1...@gmail.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I read it and they look identical. What's the > difference > > > > between > > > > > > > > them? > > > > > > > > > > > > > > > > > > > > Current avro,json, and protobuf schemas are all > implemented > > > > based > > > > > > on > > > > > > > > AVRO. > > > > > > > > > > > What do you mean, they are all implemented based on > Avro? > > > You > > > > > > mean > > > > > > > > the > > > > > > > > > > > protobuf schema is converted into an Avro Schema, and > > then > > > > you > > > > > > use > > > > > > > > Avro > > > > > > > > > > > compatibility validation? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > `org.apache.pulsar.broker.service.schema.ProtobufSchemaCompatibilityCheck` > > > > > > > > > > > > > > > > > > > > > > `org.apache.pulsar.broker.service.schema.AvroSchemaCompatibilityCheck` > > > > > > > > > > > > > > > > > > > > > > `org.apache.pulsar.broker.service.schema.JsonSchemaCompatibilityCheck` > > > > > > > > > > They all extends `AvroSchemaBasedCompatibilityCheck`, the > > > > > > > > > > `checkCompatible()` is the same implementation with > `AVRO`. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Can you please explain how a Protobuf Schema descriptor can > > be > > > > > > > validated > > > > > > > > > for backward compatibility check using Avro based > > compatibility > > > > > > rules? > > > > > > > > > Doesn't it expect the schema to be Avro, but it is > actually a > > > > > > Protobuf > > > > > > > > > descriptor? > > > > > > > > > Is there some translation happening? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I think you should structure the validation rules > > > differently: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The Compatibility check strategy is described on the > > website > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://pulsar.apache.org/docs/next/schema-understand/#schema-compatibility-check-strategy > > > > > > > > > > 1. BACKWARD(CanReadExistingStrategy): Consumers using > > schema > > > > V3 can > > > > > > > > process > > > > > > > > > > data written by producers using the last schema version > V2. > > > So > > > > V2 > > > > > > is > > > > > > > > > > "writtenSchema" and V3 is "readSchema". > > > > > > > > > > 2. FORWARD(CanBeReadByExistingStrategy): Consumers using > > the > > > > last > > > > > > > > schema > > > > > > > > > > version V2 can process data written by producers using a > > new > > > > schema > > > > > > > V3, > > > > > > > > > > even though they may not be able to use the full > > capabilities > > > > of > > > > > > the > > > > > > > > new > > > > > > > > > > schema. So V3 is "writtenSchema" and V2 is "readSchema". > > > > > > > > > > 3. FULL(CanBeReadMutualStrategy): Schemas are both > backward > > > and > > > > > > > forward > > > > > > > > > > compatible. > > > > > > > > > > Schema can evolve. The old version schema and the new > > version > > > > > > schema > > > > > > > > should > > > > > > > > > > be well understood. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I'm sorry - I don't understand. > > > > > > > > > I understand the different compatibility check strategies. > > > > > > > > > If you just spell them out here, then as you say, just > > > translate > > > > the > > > > > > > > > Protobuf Descriptor into an Avro schema and run the Avro > > > > > > > > > compatibility validation, no? > > > > > > > > > I believe the answer is no, since you may want to verify > > > > different > > > > > > > things > > > > > > > > > when it comes to Protobuf, which are different then Avro. > > > > > > > > > > > > > > > > > > At the current state, I can't understand your design at > all. > > > > Please > > > > > > > help > > > > > > > > > clarify that. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > So each strategy should have its own section. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The arguments of `canRead()` are writtenSchema and > > > readSchema. > > > > As > > > > > > > we've > > > > > > > > > > just described, we only need to change the order of > > arguments > > > > we > > > > > > pass > > > > > > > > to > > > > > > > > > > `canRead()`. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > sinan > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Asaf Mesika <asaf.mes...@gmail.com> 于2023年2月27日周一 > 20:49写道： > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > And you can see the difference between ProtoBuf and > > > > > > > ProtoBufNative: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://pulsar.apache.org/docs/next/schema-get-started/#protobufnative > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://pulsar.apache.org/docs/next/schema-get-started/#protobuf > > > > > > > > > > > > > > > > > > > > > > > I read it and they look identical. What's the > difference > > > > between > > > > > > > > them? > > > > > > > > > > > > > > > > > > > > > > Current avro,json, and protobuf schemas are all > > implemented > > > > based > > > > > > > on > > > > > > > > > > AVRO. > > > > > > > > > > > > > > > > > > > > > > What do you mean, they are all implemented based on > Avro? > > > You > > > > > > mean > > > > > > > > the > > > > > > > > > > > protobuf schema is converted into an Avro Schema, and > > then > > > > you > > > > > > use > > > > > > > > Avro > > > > > > > > > > > compatibility validation? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > *Here are the basic compatibility rules we've > defined:* > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I think you should structure the validation rules > > > > differently: > > > > > > > > > > > > > > > > > > > > > > * Backward checks > > > > > > > > > > > ** List down rules, where use newSchema (the schema > used > > by > > > > > > > producer > > > > > > > > or > > > > > > > > > > > consumer) and existingSchema (last schema used) > > > > > > > > > > > * Forward > > > > > > > > > > > ** List down rules, where use newSchema (the schema > used > > by > > > > > > > producer > > > > > > > > or > > > > > > > > > > > consumer) and existingSchema (last schema used) > > > > > > > > > > > > > > > > > > > > > > So each strategy should have its own section. > > > > > > > > > > > > > > > > > > > > > > I'm saying this since you used "writttenSchema" word > but > > it > > > > > > > > represents > > > > > > > > > > > something completely different if it's backward or > > forward > > > > check. > > > > > > > > > > > > > > > > > > > > > > Once you'll have that structure like that, I personally > > > will > > > > be > > > > > > > able > > > > > > > > to > > > > > > > > > > > read and understand it. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The motivation and problem statement are now good - > > thanks > > > > for > > > > > > > > improving > > > > > > > > > > > it. > > > > > > > > > > > > > > > > > > > > > > On Mon, Feb 27, 2023 at 8:20 AM SiNan Liu < > > > > > > liusinan1...@gmail.com> > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > Hi! I updated the PIP issue again. This time I've > added > > > > some > > > > > > > > background > > > > > > > > > > > and > > > > > > > > > > > > some explanations. > > > > > > > > > > > > > > > > > > > > > > > > The compatibility check rules are already written in > > the > > > > > > > > > > Implementation. > > > > > > > > > > > > ProtoBufNative implements the same canRead method as > > > Apache > > > > > > Avro. > > > > > > > > > > > > It does this by checking whether the schema for > writing > > > and > > > > > > > reading > > > > > > > > is > > > > > > > > > > > > compatible. I also indicate whether the writtenSchema > > and > > > > > > > > readSchema of > > > > > > > > > > > the > > > > > > > > > > > > Backward, Forward, and Full strategies are the old or > > the > > > > new > > > > > > > > version > > > > > > > > > > of > > > > > > > > > > > > the schema. > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > sinan > > > > > > > > > > > > > > > > > > > > > > > > Asaf Mesika <asaf.mes...@gmail.com> 于2023年2月26日周日 > > > 23:24写道： > > > > > > > > > > > > > > > > > > > > > > > > > I'm sorry, but this PIP lacks a lot of background > > > > knowledge, > > > > > > so > > > > > > > > you > > > > > > > > > > > need > > > > > > > > > > > > to > > > > > > > > > > > > > add IMO for people to understand it. You don't need > > to > > > > > > explain > > > > > > > > the > > > > > > > > > > > entire > > > > > > > > > > > > > pulsar in this PIP, but at the very least a few > > > > paragraphs > > > > > > > > detailing > > > > > > > > > > > all > > > > > > > > > > > > > you need to know, to put you in context: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > - Start by saying Pulsar as a built-in schema > > > registry > > > > > > > inside > > > > > > > > > > Pulsar > > > > > > > > > > > > > broker. > > > > > > > > > > > > > - Every time the client updates the schema, > it > > > > uploads > > > > > > it > > > > > > > > to > > > > > > > > > > the > > > > > > > > > > > > > broker. When that happens, it has a feature > > which > > > > > > > validates > > > > > > > > if > > > > > > > > > > > the > > > > > > > > > > > > > new > > > > > > > > > > > > > schema version is compatible with the > previous > > > > > > versions. > > > > > > > > There > > > > > > > > > > > > > are 4 types > > > > > > > > > > > > > of compatibility: Full, ... (complete and > > explain > > > > each > > > > > > > one > > > > > > > > > > > briefly) > > > > > > > > > > > > > - Also explain Pulsar Schema registry supports > > > various > > > > > > > schema > > > > > > > > > > > > > protocols: Avro, protobuf native, ... (complete > > the > > > > > > rest), > > > > > > > > each > > > > > > > > > > > > > protocol > > > > > > > > > > > > > has a schema which dictates how to serialize and > > > > > > deserialize > > > > > > > > the > > > > > > > > > > > > message > > > > > > > > > > > > > content into typed object. > > > > > > > > > > > > > - Explain in short what is protobuf native > > (compare > > > > > > protobuf > > > > > > > > > > > > non-native) > > > > > > > > > > > > > - Please don't paste code instead of explaining. > > > > > > > > > > > > > - Explain that protobuf native current > > validation > > > > check > > > > > > > is > > > > > > > > only > > > > > > > > > > > > > composed of checking the root message name is > > the > > > > same > > > > > > > > between > > > > > > > > > > > > > the current > > > > > > > > > > > > > schema version and the new version. > > > > > > > > > > > > > - Explain briefly what is a root message > and > > > its > > > > > > name. > > > > > > > > > > > > > - Explain the problem (list scenarios) that > we > > > have > > > > > > > because > > > > > > > > > > > > protobuf > > > > > > > > > > > > > native schema only supports FULL > compatibility > > > > > > > validation. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Regarding high level design - as in what you plan > to > > > do. > > > > > > > > > > > > > I suggest you add "High Level Design" and in it > > detail > > > > how > > > > > > you > > > > > > > > plan > > > > > > > > > > to > > > > > > > > > > > > > validate, per protobuf version, per compatibility > > check > > > > > > > > (backward, > > > > > > > > > > > > forward, > > > > > > > > > > > > > full,...). > > > > > > > > > > > > > I tried reading the implementation - for me , it's > > all > > > > over > > > > > > the > > > > > > > > > > place. > > > > > > > > > > > > Can > > > > > > > > > > > > > you please list in order what I wrote above, and > list > > > the > > > > > > > > validation > > > > > > > > > > > > rules > > > > > > > > > > > > > with a good explanation why you validate it like > > that? > > > > > > > > > > > > > > > > > > > > > > > > > > Lastly, one you have all the validation rules > clearly > > > > stated, > > > > > > > you > > > > > > > > can > > > > > > > > > > > use > > > > > > > > > > > > > it to document it properly so users can know what > > > > validation > > > > > > to > > > > > > > > > > expect. > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > > > > > > > > > > > Asaf > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Feb 22, 2023 at 5:10 PM SiNan Liu < > > > > > > > > liusinan1...@gmail.com> > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > Sorry, my mistake. I removed the code and > described > > > the > > > > > > > design > > > > > > > > to > > > > > > > > > > > > improve > > > > > > > > > > > > > > the PROTOBUF_NATIVE schema compatibility checks. > > You > > > > can > > > > > > > have a > > > > > > > > > > look. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Asaf Mesika <asaf.mes...@gmail.com> > 于2023年2月22日周三 > > > > 21:16写道： > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I read it but you're almost directly diving > into > > > the > > > > > > code - > > > > > > > > it > > > > > > > > > > will > > > > > > > > > > > > > take > > > > > > > > > > > > > > me > > > > > > > > > > > > > > > hours just to reverse engineer your design. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Can you please include a "High Level Design" > > > section > > > > in > > > > > > > which > > > > > > > > you > > > > > > > > > > > > > explain > > > > > > > > > > > > > > > how you plan to tackle any issue? > > > > > > > > > > > > > > > If I can read that section and explain to > someone > > > > else > > > > > > how > > > > > > > > this > > > > > > > > > > > will > > > > > > > > > > > > > > work, > > > > > > > > > > > > > > > it means the section is complete. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Let's leave the code to the PRs. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Sun, Feb 19, 2023 at 2:59 PM SiNan Liu < > > > > > > > > > > liusinan1...@gmail.com> > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
