You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@avro.apache.org by GitBox <gi...@apache.org> on 2022/07/01 17:06:12 UTC
[GitHub] [avro] RyanSkraba commented on a diff in pull request #1719: AVRO-3531:datum reader thread safe
RyanSkraba commented on code in PR #1719:
URL: https://github.com/apache/avro/pull/1719#discussion_r912121582
##########
lang/java/avro/src/main/java/org/apache/avro/generic/GenericDatumReader.java:
##########
@@ -498,34 +498,86 @@ protected Class findStringClass(Schema schema) {
}
}
- private Map<Schema, Class> stringClassCache = new IdentityHashMap<>();
+ /**
+ * This class is used to reproduce part of IdentityHashMap in ConcurrentHashMap
+ * code.
+ */
+ private static final class IdentitySchemaKey {
+ private final Schema schema;
+
+ private final int hashcode;
+
+ public IdentitySchemaKey(Schema schema) {
+ this.schema = schema;
+ this.hashcode = System.identityHashCode(schema);
+ }
- private Class getStringClass(Schema s) {
- Class c = stringClassCache.get(s);
- if (c == null) {
- c = findStringClass(s);
- stringClassCache.put(s, c);
+ @Override
+ public int hashCode() {
+ return this.hashcode;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (obj == null || !(obj instanceof GenericDatumReader.IdentitySchemaKey)) {
+ return false;
+ }
+ IdentitySchemaKey key = (IdentitySchemaKey) obj;
+ return this == key || this.schema == key.schema;
}
- return c;
}
- private final Map<Class, Constructor> stringCtorCache = new HashMap<>();
+ protected static class ReaderCache {
+ private final Map<IdentitySchemaKey, Class> stringClassCache = new ConcurrentHashMap<>();
- @SuppressWarnings("unchecked")
- protected Object newInstanceFromString(Class c, String s) {
- try {
- Constructor ctor = stringCtorCache.get(c);
- if (ctor == null) {
+ private final Map<Class, Function<String, Object>> stringCtorCache = new ConcurrentHashMap<>();
+
+ private final Function<Schema, Class> findStringClass;
+
+ public ReaderCache(Function<Schema, Class> findStringClass) {
+ this.findStringClass = findStringClass;
+ }
+
+ public Object newInstanceFromString(Class c, String s) {
+ final Function<String, Object> ctor = stringCtorCache.computeIfAbsent(c, this::buildFunction);
+ return ctor.apply(s);
+ }
+
+ private Function<String, Object> buildFunction(Class c) {
+ final Constructor ctor;
+ try {
ctor = c.getDeclaredConstructor(String.class);
- ctor.setAccessible(true);
- stringCtorCache.put(c, ctor);
+ } catch (NoSuchMethodException e) {
+ throw new AvroRuntimeException(e);
}
- return ctor.newInstance(s);
- } catch (NoSuchMethodException | InvocationTargetException | IllegalAccessException | InstantiationException e) {
- throw new AvroRuntimeException(e);
+ ctor.setAccessible(true);
+
+ return (String s) -> {
+ try {
+ return ctor.newInstance(s);
+ } catch (ReflectiveOperationException e) {
+ throw new AvroRuntimeException(e);
+ }
+ };
+ }
+
+ public Class getStringClass(final Schema s) {
+ final IdentitySchemaKey key = new IdentitySchemaKey(s);
+ return this.stringClassCache.computeIfAbsent(key, (IdentitySchemaKey k) -> this.findStringClass.apply(k.schema));
}
}
+ private final ReaderCache readerCache = new ReaderCache(this::findStringClass);
+
+ protected ReaderCache getReaderCache() {
Review Comment:
```suggestion
// VisibleForTesting
ReaderCache getReaderCache() {
```
##########
lang/java/avro/src/main/java/org/apache/avro/reflect/ReflectDatumReader.java:
##########
@@ -267,8 +267,8 @@ protected void readField(Object record, Field field, Object oldDatum, ResolvingD
if (accessor.isStringable()) {
try {
String asString = (String) read(null, field.schema(), in);
- accessor.set(record,
- asString == null ? null : newInstanceFromString(accessor.getField().getType(), asString));
+ accessor.set(record, asString == null ? null
Review Comment:
```suggestion
accessor.set(record,
asString == null ? null : newInstanceFromString(accessor.getField().getType(), asString));
```
##########
lang/java/avro/src/main/java/org/apache/avro/generic/GenericDatumReader.java:
##########
@@ -498,34 +498,86 @@ protected Class findStringClass(Schema schema) {
}
}
- private Map<Schema, Class> stringClassCache = new IdentityHashMap<>();
+ /**
+ * This class is used to reproduce part of IdentityHashMap in ConcurrentHashMap
+ * code.
+ */
+ private static final class IdentitySchemaKey {
+ private final Schema schema;
+
+ private final int hashcode;
+
+ public IdentitySchemaKey(Schema schema) {
+ this.schema = schema;
+ this.hashcode = System.identityHashCode(schema);
+ }
- private Class getStringClass(Schema s) {
- Class c = stringClassCache.get(s);
- if (c == null) {
- c = findStringClass(s);
- stringClassCache.put(s, c);
+ @Override
+ public int hashCode() {
+ return this.hashcode;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (obj == null || !(obj instanceof GenericDatumReader.IdentitySchemaKey)) {
+ return false;
+ }
+ IdentitySchemaKey key = (IdentitySchemaKey) obj;
+ return this == key || this.schema == key.schema;
}
- return c;
}
- private final Map<Class, Constructor> stringCtorCache = new HashMap<>();
+ protected static class ReaderCache {
Review Comment:
```suggestion
// VisibleForTesting
static class ReaderCache {
```
##########
lang/java/avro/src/main/java/org/apache/avro/reflect/ReflectDatumReader.java:
##########
@@ -267,8 +267,8 @@ protected void readField(Object record, Field field, Object oldDatum, ResolvingD
if (accessor.isStringable()) {
try {
String asString = (String) read(null, field.schema(), in);
- accessor.set(record,
- asString == null ? null : newInstanceFromString(accessor.getField().getType(), asString));
+ accessor.set(record, asString == null ? null
Review Comment:
This change is no longer necessary, thanks to restoring the original method.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org