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

[refactor][schema] replace linkedin/goavro/v2 with hamba/avro/v2 #1230

Merged
merged 2 commits into from
Jul 3, 2024

Conversation

adrianiacobghiula
Copy link
Contributor

@adrianiacobghiula adrianiacobghiula commented Jun 26, 2024

Motivation

Allow direct deserialization in go struct, allow generation of go struct using avro schema <- support offered by hamba/avro
Improved support in hamba avro project, while linkedin has limited support since 2022

Modifications

Replaced linkedin/goavro/v2 with hamba/avro/v2

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as (please describe tests).

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (yes)
  • The public API: (no)
  • The schema: ( no )
  • The default values of configurations: (no)
  • The wire protocol: (no)

Documentation

  • Does this pull request introduce a new feature? (yes)
  • If yes, how is the feature documented? (not applicable / docs / GoDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@adrianiacobghiula
Copy link
Contributor Author

Starting from #1191

pulsar/schema.go Outdated Show resolved Hide resolved
@nodece
Copy link
Member

nodece commented Jun 28, 2024

Rethinking recent changes, due to you changing some method parameters, it appears that there is a breaking change, which resulted in me making an incorrect review.

In the old design, the codec was exported in the schema, and which was created by NewSchemaDefinition(should be internal).

Usually, the users use the following methods to create the avro schema:

NewAvroSchema()
NewAvroSchemaWithValidation()

So that we can remove NewSchemaDefinition and AvroCodec.

The next version is the minor version, we can make this changes.

BTW, newXXSchema method returns an implement, not an interface, we also need to improve that in the future.

Is this a fair reason?

@adrianiacobghiula
Copy link
Contributor Author

@nodece fair & done

@RobertIndie RobertIndie changed the title [avro] replace linkedin/goavro/v2 with hamba/avro/v2 [refactor][schema] replace linkedin/goavro/v2 with hamba/avro/v2 Jul 3, 2024
@nodece nodece merged commit 63ae154 into apache:master Jul 3, 2024
@RobertIndie RobertIndie added this to the v0.13.0 milestone Jul 15, 2024
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.

4 participants