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

Return new ReflectData for both reflect datum reader and writer #28280

Merged
merged 2 commits into from
Sep 1, 2023

Conversation

RustedBones
Copy link
Contributor

addresses #28279

Avro has a discrepancy between reflect reader and writer side:

Updating the model data's logical types is not a thread safe operation, so we must not use the singleton instance in this case.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2023

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

@mosche mosche added the avro label Sep 1, 2023
@mosche mosche self-requested a review September 1, 2023 10:31
Copy link
Member

@mosche mosche left a comment

Choose a reason for hiding this comment

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

Interesting, constructors are not even consistent within ReflectDatumReader. Just that single constructor creates a new ReflectData instance, all the other ones use the shared instance.

Thanks a lot for addressing this inconsistency, this makes absolute sense. LGTM 💯

Though, nevertheless, I do wonder why/how this manifests as NPE at this position looking at your stack trace.

@mosche mosche merged commit 325efce into apache:master Sep 1, 2023
15 of 16 checks passed
@RustedBones RustedBones deleted the avro-reflect-data-datum-factory branch September 1, 2023 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants