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.

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

[Avro] Remove dependencies upon Jackson 1.X and Avro's JacksonUtils #195

Merged
merged 1 commit into from
Feb 29, 2020

Conversation

baharclerode
Copy link
Contributor

@baharclerode baharclerode commented Feb 21, 2020

This is the first half of a fix for #167

This removes the references to Jackson 1.x and JacksonUtils (which is internal to Avro), allowing Jackson-Avro to be binary-compatible Avro 1.9 or Avro 1.8 but not semantically-compatible in all cases, particularly around nested classes; For simple use-cases, this is sufficient to allow Jackson-Avro to work with Avro 1.9, but I would not by any means call it "supported" due to the nasty gotchas that still lurk.

This does not provide full backwards and forwards compatibility with Avro 1.9; Notably:

  • Generating a schema using Jackson-Avro w/ Avro 1.9 will still produce a 1.8 schema for nested classes; Avro 1.9 can't use these for serialization (But Jackson-Avro w/ Avro 1.9 can). Deserialization still seems to work.
  • Uses of the @AvroAlias annotation to create aliases to nested types will break, since previously these had to follow the Avro 1.8 conventions for namespacing nested types
  • Mixing of Avro 1.8 and Avro 1.9 anywhere is right out. You can't generate a schema with Avro 1.8 and have Jackson-Avro w/ Avro 1.9 do anything with it if nested classes are involved, and vice versa. The same goes for using Jackson-Avro w/ Avro 1.8 to generate schemas and Avro 1.9 to serialize/deserialize, and vice versa. Don't try it. If it works, it'll work just long enough for you to forget about it not supporting nested classes.

Fixing of the above will probably require porting all of the Avro union resolver code into Jackson-Avro with modifications to allow it to resolve both 1.8- and 1.9-styled namespaces, and a mapper flag that indicates if you want 1.8 or 1.9-compatibility for @AvroAlias annotations and generated schemas.

@cowtowncoder cowtowncoder merged commit c570549 into FasterXML:2.11 Feb 29, 2020
@cowtowncoder
Copy link
Member

Thanks! I'll make a minor post-merge change probably to isolate use of ObjectMapper, but other than that it's the first big step.

// Check if this is a nested class
String nestedClassName = sb.append(namespace).append('$').append(name).toString();
try {
Class.forName(nestedClassName);
Copy link
Member

Choose a reason for hiding this comment

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

@baharclerode assuming most common case is NOT inner class, would it make sense to try loading "regular" class first, inverting logic? Only asking since there's quite a bit of overhead for exception handling and possibly other corner cases for trying to load non-existing class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so; Either way, we're always doing at least 1 Class.forName(); Either we check for the non-nested class, and assume if that fails that it's nested, or we always check for the nested class, and if that fails, assume that it's not-nested.

Copy link
Member

Choose a reason for hiding this comment

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

Alas, looks like this lead to a major performance issue for some usage (see #219 for attempts to work around the issue). I assume that case of finding class is relatively cheap, but that failure is enormously expensive (as that is the "rare" case being optimized against), depending on complexity of classpath for application.

cowtowncoder added a commit that referenced this pull request Feb 29, 2020
@cowtowncoder
Copy link
Member

Merged; did some minor renaming.

@cowtowncoder
Copy link
Member

Turns out I had to do some more tweaking in 3.0, to avoid breaking #145. Not sure if similar "null masking" might be needed for 2.x, come to think of that (that is, when converting from NullNode, should use JsonProperties.NULL_VALUE, to retain "explicit null as default" vs "no default value).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants