[GitHub] [pulsar] eolivelli commented on a change in pull request #10428: [Issue #10427] Add AvroSchema UUID support fix

[GitBox]

eolivelli commented on a change in pull request #10428:
URL: https://github.com/apache/pulsar/pull/10428#discussion_r635953777



##########
File path: 
pulsar-client/src/main/java/org/apache/pulsar/client/impl/schema/util/SchemaUtil.java
##########
@@ -88,8 +90,10 @@ public static Schema extractAvroSchema(SchemaDefinition 
schemaDefinition, Class
         try {
             return 
parseAvroSchema(pojo.getDeclaredField("SCHEMA$").get(null).toString());
         } catch (NoSuchFieldException | IllegalAccessException | 
IllegalArgumentException ignored) {
-            return schemaDefinition.getAlwaysAllowNull() ? 
ReflectData.AllowNull.get().getSchema(pojo)
-                    : ReflectData.get().getSchema(pojo);
+            ReflectData reflectData = schemaDefinition.getAlwaysAllowNull() ? 
ReflectData.AllowNull.get()
+                    : ReflectData.get();
+            AvroSchema.addLogicalTypeConversions(reflectData, 
schemaDefinition.isJsr310ConversionEnabled());

Review comment:
       @vroyer 
   I share @sijie's concern.
   I didn't notice that we are modifying a singleton.
   we could initialise it only once 
   
   but I cannot understand how it can work with 
`schemaDefinition.isJsr310ConversionEnabled())`, that depends on a particular 
schema.
   if we have two schemas with different values for isJsr310ConversionEnabled, 
we will have a race and it may happen that we use the wrong configuration 
(redarding isJsr310ConversionEnabled) 




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