lragnarsson commented on a change in pull request #1320:
URL: https://github.com/apache/avro/pull/1320#discussion_r699083711



##########
File path: 
lang/java/protobuf/src/main/java/org/apache/avro/protobuf/ProtobufData.java
##########
@@ -113,17 +114,17 @@ protected Object getField(Object record, String name, int 
pos, Object state) {
     }
   }
 
-  private final Map<Descriptor, FieldDescriptor[]> fieldCache = new 
ConcurrentHashMap<>();
+  private final Map<DescriptorProto, FieldDescriptor[]> fieldCache = new 
ConcurrentHashMap<>();
 
   @Override
   protected Object getRecordState(Object r, Schema s) {
     Descriptor d = ((MessageOrBuilder) r).getDescriptorForType();
-    FieldDescriptor[] fields = fieldCache.get(d);
+    FieldDescriptor[] fields = fieldCache.get(d.toProto());
     if (fields == null) { // cache miss
       fields = new FieldDescriptor[s.getFields().size()];
       for (Field f : s.getFields())
         fields[f.pos()] = d.findFieldByName(f.name());
-      fieldCache.put(d, fields); // update cache
+      fieldCache.put(d.toProto(), fields); // update cache

Review comment:
       This is probably a good idea, internally, toProto() just does "return 
this.proto" but that is an implementation that might change.

##########
File path: 
lang/java/protobuf/src/main/java/org/apache/avro/protobuf/ProtobufData.java
##########
@@ -191,26 +192,27 @@ public Schema getSchema(Class c) {
     return schema;
   }
 
-  private static final ThreadLocal<Map<Descriptor, Schema>> SEEN = 
ThreadLocal.withInitial(IdentityHashMap::new);
+  private static final ThreadLocal<Map<DescriptorProto, Schema>> SEEN = 
ThreadLocal.withInitial(IdentityHashMap::new);
 
   public Schema getSchema(Descriptor descriptor) {
-    Map<Descriptor, Schema> seen = SEEN.get();
-    if (seen.containsKey(descriptor)) // stop recursion
-      return seen.get(descriptor);
+    Map<DescriptorProto, Schema> seen = SEEN.get();
+    DescriptorProto dp = descriptor.toProto();
+    if (seen.containsKey(dp)) // stop recursion
+      return seen.get(dp);

Review comment:
       Makes sense, I didn't want to change too much of the current 
implementation though.




-- 
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]


Reply via email to