Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cache record names to avoid hitting class loader #219

Merged
merged 2 commits into from
Sep 19, 2020
Merged

Cache record names to avoid hitting class loader #219

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
887f2db
Cache record names to avoid hitting class loader
marcospassos Sep 11, 2020
db1b79d
Use LRUCache instead of HashMap
marcospassos Sep 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 50 additions & 25 deletions avro/src/main/java/com/fasterxml/jackson/dataformat/avro/schema/AvroSchemaHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.fasterxml.jackson.dataformat.avro.schema;

import com.fasterxml.jackson.databind.util.LRUMap;
import java.io.File;
import java.math.BigDecimal;
import java.math.BigInteger;
Expand All @@ -24,6 +25,8 @@

public abstract class AvroSchemaHelper
{
private static final LRUMap<String, String> SCHEMA_NAME_CACHE = new LRUMap<>(16, 1024);

/**
* Dedicated mapper for handling default values (String &lt;-&gt; JsonNode &lt;-&gt; Object)
*
Expand Down Expand Up @@ -332,32 +335,54 @@ public static String getTypeId(Schema schema) {
*/
public static String getFullName(Schema schema) {
switch (schema.getType()) {
case RECORD:
case ENUM:
case FIXED:
String namespace = schema.getNamespace();
String name = schema.getName();
if (namespace == null) {
return schema.getName();
}
if (namespace.endsWith("$")) {
return namespace + name;
}
StringBuilder sb = new StringBuilder(1 + namespace.length() + name.length());
// 28-Feb-2020: [dataformats-binary#195] somewhat complicated logic of trying
// to support differences between avro-lib 1.8 and 1.9...
// Check if this is a nested class
String nestedClassName = sb.append(namespace).append('$').append(name).toString();
try {
Class.forName(nestedClassName);
return nestedClassName;
} catch (ClassNotFoundException e) {
// Could not find a nested class, must be a regular class
sb.setLength(0);
return sb.append(namespace).append('.').append(name).toString();
}
case RECORD:
case ENUM:
case FIXED:
String namespace = schema.getNamespace();
String name = schema.getName();
String key = namespace + "." + name;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

namespace might be null here (not when a schema is generated from a java class, but if using hand-crafted or schemas generated from avdl files)

Copy link
Contributor

@baharclerode baharclerode Sep 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just double checked, schema.getFullName() should be suitable as a cache key here without any additional checks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi there @baharclerode! Thank you for additional comments -- and yes, I realize that Avro's choice of "interesting" ways of dealing with stuff sort of forces our hand here.
Would be happy to merge a PR for removing helper class as key, use schema.getFullName() if that should work?

One other related question: if we had a setting to select between "1.8-and-earlier" vs "1.9-and-later" modes, would it be possible to use different logic, avoiding Class lookup or at least alleviate it?
Or does the problem exist regardless of this, with 1.9 and above?
(I realize that right now lack of context makes it impossible to have such a setting, but I still hope we can make AvroSchemaHelper become non-static and/or take some context object we control which could pass configuration, and ideally contain per-mapper (or per-schema or whatever) cache).

String schemaName = SCHEMA_NAME_CACHE.get(key);

if (schemaName == null) {
schemaName = resolveFullName(schema);
SCHEMA_NAME_CACHE.put(key, schemaName);
}

return schemaName;

default:
return schema.getType().getName();
return schema.getType().getName();
}
}

private static String resolveFullName(Schema schema) {
String namespace = schema.getNamespace();
String name = schema.getName();

if (namespace == null) {
return schema.getName();
}

if (namespace.endsWith("$")) {
return namespace + name;
}

StringBuilder sb = new StringBuilder(1 + namespace.length() + name.length());

// 28-Feb-2020: [dataformats-binary#195] somewhat complicated logic of trying
// to support differences between avro-lib 1.8 and 1.9...
// Check if this is a nested class
String nestedClassName = sb.append(namespace).append('$').append(name).toString();

try {
Class.forName(nestedClassName);

return nestedClassName;
} catch (ClassNotFoundException e) {
// Could not find a nested class, must be a regular class
sb.setLength(0);

return sb.append(namespace).append('.').append(name).toString();
}
}

Expand Down