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

ISSUE-548: Make compatible with Avro 1.9.0 #547

Merged
merged 1 commit into from
Aug 26, 2019

Conversation

ivangreene
Copy link
Contributor

@ivangreene ivangreene commented Apr 4, 2019

Preparing for the upcoming release of Avro 1.9.0,
there are some methods and transitive dependencies
that have been removed.
Resolves #548

This still needs some work regarding default values.
As of 1.8.x, Avro's Schema.Field class exposed
'defaultValue', returning a JsonNode. That method is
package private in 1.9.0-SNAPSHOT, in favor of defaultVal
returning an Object. In this instance, we have no way to
distinguish between 'null' as a default and a field having
no default at all.
So we need to figure out how to manage
these cases. We could try to squeeze in 'hasDefault' before
the new Avro release but we will be binary incompatible at
that point so we would need 2 registry builds for Avro 1.8.x
and 1.9.0.

Mostly false alarm on the above comment. I spent a bunch of time
setting up reflective junk to mitigate what I thought was
a problem but it turned out that defaultVal() returns
JsonProperties.NULL_VALUE when a field has a default
of 'null'. So looks like we are mostly good.
The only failure against Avro 1.9.0-SNAPSHOT came when using the
Confluent schema registry in the schema-registry-webservice
tests.

@michaelandrepearce
Copy link
Collaborator

michaelandrepearce commented Apr 4, 2019

@ivangreene if a method was previously accessible and now they're removing that method, thats a breaking change in avro, maybe best to discuss in upstream avro project, if they could revert that method or at least keep it but as deperecated, so its not a hard break, before they release.

@ivangreene
Copy link
Contributor Author

@michaelandrepearce I will try to see if they will accept a patch to expose this method again. If we can’t find a way to make a registry version compatible with both 1.8 and 1.9 line this will hurt adoption of the new Avro version a lot IMO

Preparing for the upcoming release of Avro 1.9.0,
there are some methods and transitive dependencies
that have been removed.
@ivangreene ivangreene changed the title Fix compatibility with Avro 1.9.0-SNAPSHOT ISSUE-548: Make compatible with Avro 1.9.0 Apr 6, 2019
@ivangreene
Copy link
Contributor Author

@michaelandrepearce I updated my comment to reflect new findings. So I think we are mostly going to be compatible here aside from the tests using the Confluent registry, that will only need to change when this project wants to start running the CI against 1.9

@ivangreene
Copy link
Contributor Author

@satishd can you have a look at this? Avro 1.9.0 has been released so this will block users from being able to upgrade

@ivangreene
Copy link
Contributor Author

@raju-saravanan I see you just put out 0.8.0 RC1, can you have a look at this PR for inclusion? This will be nice and Avro is planning on releasing 1.9.1 soon so users will want to be able to upgrade

@raju-saravanan
Copy link
Collaborator

raju-saravanan commented Aug 23, 2019

@ivangreene: I have put 0.8.0 release on hold for now. That being said, I am ok with the changes in this PR. I will wait for a day or two for @satishd and @michaelandrepearce to get their opinion.

@raju-saravanan raju-saravanan self-requested a review August 26, 2019 06:32
Copy link
Collaborator

@raju-saravanan raju-saravanan left a comment

Choose a reason for hiding this comment

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

+1 LGTM

@raju-saravanan raju-saravanan merged commit b572f72 into hortonworks:master Aug 26, 2019
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.

Make compatible with Avro 1.9.0
3 participants