You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/02/18 07:34:56 UTC

[GitHub] [pulsar] congbobo184 opened a new pull request #9612: [Schema] Schema comparison logic change.

congbobo184 opened a new pull request #9612:
URL: https://github.com/apache/pulsar/pull/9612


   ## Motivation
   link : #8510 
   Now the logic for comparing the schema is to compare the store byte[], it can't ignore the whitespace or '/n' or other character, we should compare by schema.parse.  
   ## implement
   ### Verifying this change
   Add the test for it
   
   Does this pull request potentially affect one of the following parts:
   If yes was chosen, please highlight the changes
   
   Dependencies (does it add or upgrade a dependency): (no)
   The public API: (no)
   The schema: (no)
   The default values of configurations: (no)
   The wire protocol: (no)
   The rest endpoints: (no)
   The admin cli options: (no)
   Anything that affects deployment: (no)
   
   


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [pulsar] congbobo184 commented on pull request #9612: [Schema] Schema comparison logic change.

Posted by GitBox <gi...@apache.org>.
congbobo184 commented on pull request #9612:
URL: https://github.com/apache/pulsar/pull/9612#issuecomment-784046112


   /pulsarbot run-failure-checks


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [pulsar] zymap merged pull request #9612: [Schema] Schema comparison logic change.

Posted by GitBox <gi...@apache.org>.
zymap merged pull request #9612:
URL: https://github.com/apache/pulsar/pull/9612


   


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [pulsar] eolivelli commented on a change in pull request #9612: [Schema] Schema comparison logic change.

Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #9612:
URL: https://github.com/apache/pulsar/pull/9612#discussion_r580231401



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/SchemaRegistryServiceImpl.java
##########
@@ -467,4 +493,12 @@ static SchemaData schemaInfoToSchema(SchemaRegistryFormat.SchemaInfo info) {
         }
     }
 
+    public static boolean isUsingAvroSchemaParser(SchemaType type) {
+        if (type == SchemaType.AVRO || type == SchemaType.PROTOBUF || type == SchemaType.JSON) {

Review comment:
       nit:
   you can use a `switch` block 




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [pulsar] congbobo184 commented on pull request #9612: [Schema] Schema comparison logic change.

Posted by GitBox <gi...@apache.org>.
congbobo184 commented on pull request #9612:
URL: https://github.com/apache/pulsar/pull/9612#issuecomment-784022611


   /pulsarbot run-failure-checks


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [pulsar] congbobo184 commented on pull request #9612: [Schema] Schema comparison logic change.

Posted by GitBox <gi...@apache.org>.
congbobo184 commented on pull request #9612:
URL: https://github.com/apache/pulsar/pull/9612#issuecomment-782977998


   /pulsarbot run-failure-checks


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [pulsar] congbobo184 commented on pull request #9612: [Schema] Schema comparison logic change.

Posted by GitBox <gi...@apache.org>.
congbobo184 commented on pull request #9612:
URL: https://github.com/apache/pulsar/pull/9612#issuecomment-783968760


   /pulsarbot run-failure-checks


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [pulsar] congbobo184 commented on a change in pull request #9612: [Schema] Schema comparison logic change.

Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #9612:
URL: https://github.com/apache/pulsar/pull/9612#discussion_r580751679



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/SchemaRegistryServiceImpl.java
##########
@@ -467,4 +493,12 @@ static SchemaData schemaInfoToSchema(SchemaRegistryFormat.SchemaInfo info) {
         }
     }
 
+    public static boolean isUsingAvroSchemaParser(SchemaType type) {
+        if (type == SchemaType.AVRO || type == SchemaType.PROTOBUF || type == SchemaType.JSON) {

Review comment:
       oh yes, I will change it to switch block.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [pulsar] eolivelli commented on a change in pull request #9612: [Schema] Schema comparison logic change.

Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #9612:
URL: https://github.com/apache/pulsar/pull/9612#discussion_r580069206



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/SchemaRegistryServiceImpl.java
##########
@@ -293,12 +295,28 @@ private void checkCompatible(SchemaAndMetadata existingSchema, SchemaData newSch
             SchemaData schemaData) {
         final CompletableFuture<SchemaVersion> completableFuture = new CompletableFuture<>();
         SchemaVersion schemaVersion;
-        for (SchemaAndMetadata schemaAndMetadata : schemaAndMetadataList) {
-            if (Arrays.equals(hashFunction.hashBytes(schemaAndMetadata.schema.getData()).asBytes(),
-                    hashFunction.hashBytes(schemaData.getData()).asBytes())) {
-                schemaVersion = schemaAndMetadata.version;
-                completableFuture.complete(schemaVersion);
-                return completableFuture;
+        if (schemaData.getData().length != 0) {
+            Schema.Parser parser = new Schema.Parser();
+            Schema newSchema = parser.parse(new String(schemaData.getData(), UTF_8));

Review comment:
       very good!
   
   we are still missing PROTOBUF tests, correct ?




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [pulsar] eolivelli commented on a change in pull request #9612: [Schema] Schema comparison logic change.

Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #9612:
URL: https://github.com/apache/pulsar/pull/9612#discussion_r578352086



##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/schema/compatibility/SchemaCompatibilityCheckTest.java
##########
@@ -294,6 +298,53 @@ public void testIsAutoUpdateSchema(SchemaCompatibilityStrategy schemaCompatibili
         producer.close();
     }
 
+    @Test
+    public void testSchemaComparison() throws Exception {
+        final String tenant = PUBLIC_TENANT;
+        final String topic = "test-schema-comparison";
+
+        String namespace = "test-namespace-" + randomName(16);
+        String fqtn = TopicName.get(
+                TopicDomain.persistent.value(),
+                tenant,
+                namespace,
+                topic
+        ).toString();
+
+        NamespaceName namespaceName = NamespaceName.get(tenant, namespace);
+
+        admin.namespaces().createNamespace(
+                tenant + "/" + namespace,
+                Sets.newHashSet(CLUSTER_NAME)
+        );
+
+        assertEquals(admin.namespaces().getSchemaCompatibilityStrategy(namespaceName.toString()),
+                SchemaCompatibilityStrategy.FULL);
+        byte[] changeSchemaBytes = (new String(Schema.AVRO(Schemas.PersonOne.class)
+                .getSchemaInfo().getSchema(), UTF_8) + "/n   /n   /n").getBytes();
+        SchemaInfo schemaInfo = SchemaInfo.builder().type(SchemaType.AVRO).schema(changeSchemaBytes).build();
+        admin.schemas().createSchema(fqtn, schemaInfo);
+
+        admin.namespaces().setIsAllowAutoUpdateSchema(namespaceName.toString(), false);
+        ProducerBuilder<Schemas.PersonOne> producerOneBuilder = pulsarClient
+                .newProducer(Schema.AVRO(Schemas.PersonOne.class))
+                .topic(fqtn);
+        producerOneBuilder.create().close();
+
+        assertArrayEquals(changeSchemaBytes, admin.schemas().getSchemaInfo(fqtn).getSchema());
+
+        ProducerBuilder<Schemas.PersonThree> producerThreeBuilder = pulsarClient
+                .newProducer(Schema.AVRO(Schemas.PersonThree.class))
+                .topic(fqtn);
+        
+        try {
+            producerThreeBuilder.create();
+        } catch (Exception e) {
+            e.printStackTrace();

Review comment:
       please use logger

##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/SchemaRegistryServiceImpl.java
##########
@@ -293,12 +295,28 @@ private void checkCompatible(SchemaAndMetadata existingSchema, SchemaData newSch
             SchemaData schemaData) {
         final CompletableFuture<SchemaVersion> completableFuture = new CompletableFuture<>();
         SchemaVersion schemaVersion;
-        for (SchemaAndMetadata schemaAndMetadata : schemaAndMetadataList) {
-            if (Arrays.equals(hashFunction.hashBytes(schemaAndMetadata.schema.getData()).asBytes(),
-                    hashFunction.hashBytes(schemaData.getData()).asBytes())) {
-                schemaVersion = schemaAndMetadata.version;
-                completableFuture.complete(schemaVersion);
-                return completableFuture;
+        if (schemaData.getData().length != 0) {
+            Schema.Parser parser = new Schema.Parser();
+            Schema newSchema = parser.parse(new String(schemaData.getData(), UTF_8));

Review comment:
       are we sure that here we are dealing with an Avro schema ?
   what happens in case of a non-Avro schema ?

##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/SchemaRegistryServiceImpl.java
##########
@@ -293,12 +295,28 @@ private void checkCompatible(SchemaAndMetadata existingSchema, SchemaData newSch
             SchemaData schemaData) {
         final CompletableFuture<SchemaVersion> completableFuture = new CompletableFuture<>();
         SchemaVersion schemaVersion;
-        for (SchemaAndMetadata schemaAndMetadata : schemaAndMetadataList) {
-            if (Arrays.equals(hashFunction.hashBytes(schemaAndMetadata.schema.getData()).asBytes(),
-                    hashFunction.hashBytes(schemaData.getData()).asBytes())) {
-                schemaVersion = schemaAndMetadata.version;
-                completableFuture.complete(schemaVersion);
-                return completableFuture;
+        if (schemaData.getData().length != 0) {

Review comment:
       you added one test, which one of the two branches is covered by the new test ?
   Can you add a test for the other branch or point to an existing test ?




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [pulsar] congbobo184 commented on a change in pull request #9612: [Schema] Schema comparison logic change.

Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #9612:
URL: https://github.com/apache/pulsar/pull/9612#discussion_r578460141



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/SchemaRegistryServiceImpl.java
##########
@@ -293,12 +295,28 @@ private void checkCompatible(SchemaAndMetadata existingSchema, SchemaData newSch
             SchemaData schemaData) {
         final CompletableFuture<SchemaVersion> completableFuture = new CompletableFuture<>();
         SchemaVersion schemaVersion;
-        for (SchemaAndMetadata schemaAndMetadata : schemaAndMetadataList) {
-            if (Arrays.equals(hashFunction.hashBytes(schemaAndMetadata.schema.getData()).asBytes(),
-                    hashFunction.hashBytes(schemaData.getData()).asBytes())) {
-                schemaVersion = schemaAndMetadata.version;
-                completableFuture.complete(schemaVersion);
-                return completableFuture;
+        if (schemaData.getData().length != 0) {
+            Schema.Parser parser = new Schema.Parser();
+            Schema newSchema = parser.parse(new String(schemaData.getData(), UTF_8));

Review comment:
       this commit only test any test will not pass and then I will reimplement 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:
users@infra.apache.org



[GitHub] [pulsar] congbobo184 commented on a change in pull request #9612: [Schema] Schema comparison logic change.

Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #9612:
URL: https://github.com/apache/pulsar/pull/9612#discussion_r580754274



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/SchemaRegistryServiceImpl.java
##########
@@ -293,12 +295,36 @@ private void checkCompatible(SchemaAndMetadata existingSchema, SchemaData newSch
             SchemaData schemaData) {
         final CompletableFuture<SchemaVersion> completableFuture = new CompletableFuture<>();
         SchemaVersion schemaVersion;
-        for (SchemaAndMetadata schemaAndMetadata : schemaAndMetadataList) {
-            if (Arrays.equals(hashFunction.hashBytes(schemaAndMetadata.schema.getData()).asBytes(),
-                    hashFunction.hashBytes(schemaData.getData()).asBytes())) {
-                schemaVersion = schemaAndMetadata.version;
-                completableFuture.complete(schemaVersion);
-                return completableFuture;
+        if (isUsingAvroSchemaParser(schemaData.getType())) {
+            Schema.Parser parser = new Schema.Parser();
+            Schema newSchema = parser.parse(new String(schemaData.getData(), UTF_8));
+
+            for (SchemaAndMetadata schemaAndMetadata : schemaAndMetadataList) {
+                if (isUsingAvroSchemaParser(schemaData.getType())) {
+                    Schema.Parser existParser = new Schema.Parser();
+                    Schema existSchema = existParser.parse(new String(schemaAndMetadata.schema.getData(), UTF_8));
+                    if (newSchema.equals(existSchema)) {
+                        schemaVersion = schemaAndMetadata.version;
+                        completableFuture.complete(schemaVersion);
+                        return completableFuture;
+                    }
+                } else {
+                    if (Arrays.equals(hashFunction.hashBytes(schemaAndMetadata.schema.getData()).asBytes(),
+                            hashFunction.hashBytes(schemaData.getData()).asBytes())) {
+                        schemaVersion = schemaAndMetadata.version;
+                        completableFuture.complete(schemaVersion);
+                        return completableFuture;
+                    }

Review comment:
       schemaType don't check now, it will be fix in another pr and ```GenericProtobufNativeSchema``` will be fix in another pr. this pr only fix the avro comparison. :)
   




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [pulsar] congbobo184 removed a comment on pull request #9612: [Schema] Schema comparison logic change.

Posted by GitBox <gi...@apache.org>.
congbobo184 removed a comment on pull request #9612:
URL: https://github.com/apache/pulsar/pull/9612#issuecomment-783968760


   /pulsarbot run-failure-checks


----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [pulsar] codelipenghui commented on a change in pull request #9612: [Schema] Schema comparison logic change.

Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #9612:
URL: https://github.com/apache/pulsar/pull/9612#discussion_r580227922



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/SchemaRegistryServiceImpl.java
##########
@@ -293,12 +295,36 @@ private void checkCompatible(SchemaAndMetadata existingSchema, SchemaData newSch
             SchemaData schemaData) {
         final CompletableFuture<SchemaVersion> completableFuture = new CompletableFuture<>();
         SchemaVersion schemaVersion;
-        for (SchemaAndMetadata schemaAndMetadata : schemaAndMetadataList) {
-            if (Arrays.equals(hashFunction.hashBytes(schemaAndMetadata.schema.getData()).asBytes(),
-                    hashFunction.hashBytes(schemaData.getData()).asBytes())) {
-                schemaVersion = schemaAndMetadata.version;
-                completableFuture.complete(schemaVersion);
-                return completableFuture;
+        if (isUsingAvroSchemaParser(schemaData.getType())) {
+            Schema.Parser parser = new Schema.Parser();
+            Schema newSchema = parser.parse(new String(schemaData.getData(), UTF_8));
+
+            for (SchemaAndMetadata schemaAndMetadata : schemaAndMetadataList) {
+                if (isUsingAvroSchemaParser(schemaData.getType())) {
+                    Schema.Parser existParser = new Schema.Parser();
+                    Schema existSchema = existParser.parse(new String(schemaAndMetadata.schema.getData(), UTF_8));
+                    if (newSchema.equals(existSchema)) {
+                        schemaVersion = schemaAndMetadata.version;
+                        completableFuture.complete(schemaVersion);
+                        return completableFuture;
+                    }
+                } else {
+                    if (Arrays.equals(hashFunction.hashBytes(schemaAndMetadata.schema.getData()).asBytes(),
+                            hashFunction.hashBytes(schemaData.getData()).asBytes())) {
+                        schemaVersion = schemaAndMetadata.version;
+                        completableFuture.complete(schemaVersion);
+                        return completableFuture;
+                    }

Review comment:
       return false directly? If the new schema is using Avro parse but the existing schema is not using the Avro parse, these two schemas cannot be the same schema?




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [pulsar] congbobo184 commented on a change in pull request #9612: [Schema] Schema comparison logic change.

Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #9612:
URL: https://github.com/apache/pulsar/pull/9612#discussion_r580087259



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/SchemaRegistryServiceImpl.java
##########
@@ -293,12 +295,28 @@ private void checkCompatible(SchemaAndMetadata existingSchema, SchemaData newSch
             SchemaData schemaData) {
         final CompletableFuture<SchemaVersion> completableFuture = new CompletableFuture<>();
         SchemaVersion schemaVersion;
-        for (SchemaAndMetadata schemaAndMetadata : schemaAndMetadataList) {
-            if (Arrays.equals(hashFunction.hashBytes(schemaAndMetadata.schema.getData()).asBytes(),
-                    hashFunction.hashBytes(schemaData.getData()).asBytes())) {
-                schemaVersion = schemaAndMetadata.version;
-                completableFuture.complete(schemaVersion);
-                return completableFuture;
+        if (schemaData.getData().length != 0) {
+            Schema.Parser parser = new Schema.Parser();
+            Schema newSchema = parser.parse(new String(schemaData.getData(), UTF_8));

Review comment:
       PROTOBUF also use avro struct to store. So, we not need to add PROTOBUF tests




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [pulsar] eolivelli commented on a change in pull request #9612: [Schema] Schema comparison logic change.

Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #9612:
URL: https://github.com/apache/pulsar/pull/9612#discussion_r580090240



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/SchemaRegistryServiceImpl.java
##########
@@ -293,12 +295,28 @@ private void checkCompatible(SchemaAndMetadata existingSchema, SchemaData newSch
             SchemaData schemaData) {
         final CompletableFuture<SchemaVersion> completableFuture = new CompletableFuture<>();
         SchemaVersion schemaVersion;
-        for (SchemaAndMetadata schemaAndMetadata : schemaAndMetadataList) {
-            if (Arrays.equals(hashFunction.hashBytes(schemaAndMetadata.schema.getData()).asBytes(),
-                    hashFunction.hashBytes(schemaData.getData()).asBytes())) {
-                schemaVersion = schemaAndMetadata.version;
-                completableFuture.complete(schemaVersion);
-                return completableFuture;
+        if (schemaData.getData().length != 0) {
+            Schema.Parser parser = new Schema.Parser();
+            Schema newSchema = parser.parse(new String(schemaData.getData(), UTF_8));

Review comment:
       Sorry I was not clear.
   My comment is more about having code coverage for the condition about PROTOBUF.
   if we add an explicit unit test about the fact that with PROTOBUF we are performing your new code, we are sure that we won't lose this implementation in the future.
   
   Probably adding an utility method `isUsingAvroSchemaParser` and add simple unit tests about that method will reduce the number of combinations we need to completely cover this change.
   
   




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [pulsar] congbobo184 commented on a change in pull request #9612: [Schema] Schema comparison logic change.

Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #9612:
URL: https://github.com/apache/pulsar/pull/9612#discussion_r578460141



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/SchemaRegistryServiceImpl.java
##########
@@ -293,12 +295,28 @@ private void checkCompatible(SchemaAndMetadata existingSchema, SchemaData newSch
             SchemaData schemaData) {
         final CompletableFuture<SchemaVersion> completableFuture = new CompletableFuture<>();
         SchemaVersion schemaVersion;
-        for (SchemaAndMetadata schemaAndMetadata : schemaAndMetadataList) {
-            if (Arrays.equals(hashFunction.hashBytes(schemaAndMetadata.schema.getData()).asBytes(),
-                    hashFunction.hashBytes(schemaData.getData()).asBytes())) {
-                schemaVersion = schemaAndMetadata.version;
-                completableFuture.complete(schemaVersion);
-                return completableFuture;
+        if (schemaData.getData().length != 0) {
+            Schema.Parser parser = new Schema.Parser();
+            Schema newSchema = parser.parse(new String(schemaData.getData(), UTF_8));

Review comment:
       yes, this commit only test any test will not pass and then I will reimplement 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:
users@infra.apache.org



[GitHub] [pulsar] congbobo184 commented on a change in pull request #9612: [Schema] Schema comparison logic change.

Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #9612:
URL: https://github.com/apache/pulsar/pull/9612#discussion_r580122192



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/SchemaRegistryServiceImpl.java
##########
@@ -293,12 +295,28 @@ private void checkCompatible(SchemaAndMetadata existingSchema, SchemaData newSch
             SchemaData schemaData) {
         final CompletableFuture<SchemaVersion> completableFuture = new CompletableFuture<>();
         SchemaVersion schemaVersion;
-        for (SchemaAndMetadata schemaAndMetadata : schemaAndMetadataList) {
-            if (Arrays.equals(hashFunction.hashBytes(schemaAndMetadata.schema.getData()).asBytes(),
-                    hashFunction.hashBytes(schemaData.getData()).asBytes())) {
-                schemaVersion = schemaAndMetadata.version;
-                completableFuture.complete(schemaVersion);
-                return completableFuture;
+        if (schemaData.getData().length != 0) {
+            Schema.Parser parser = new Schema.Parser();
+            Schema newSchema = parser.parse(new String(schemaData.getData(), UTF_8));

Review comment:
       good idea, i will add the method ```isUsingAvroSchemaParser``` and add test for it.




----------------------------------------------------------------
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:
users@infra.apache.org



[GitHub] [pulsar] congbobo184 commented on a change in pull request #9612: [Schema] Schema comparison logic change.

Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #9612:
URL: https://github.com/apache/pulsar/pull/9612#discussion_r580087259



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/schema/SchemaRegistryServiceImpl.java
##########
@@ -293,12 +295,28 @@ private void checkCompatible(SchemaAndMetadata existingSchema, SchemaData newSch
             SchemaData schemaData) {
         final CompletableFuture<SchemaVersion> completableFuture = new CompletableFuture<>();
         SchemaVersion schemaVersion;
-        for (SchemaAndMetadata schemaAndMetadata : schemaAndMetadataList) {
-            if (Arrays.equals(hashFunction.hashBytes(schemaAndMetadata.schema.getData()).asBytes(),
-                    hashFunction.hashBytes(schemaData.getData()).asBytes())) {
-                schemaVersion = schemaAndMetadata.version;
-                completableFuture.complete(schemaVersion);
-                return completableFuture;
+        if (schemaData.getData().length != 0) {
+            Schema.Parser parser = new Schema.Parser();
+            Schema newSchema = parser.parse(new String(schemaData.getData(), UTF_8));

Review comment:
       PROTOBUF also use avro struct to store.




----------------------------------------------------------------
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:
users@infra.apache.org