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

[schema-registry] Implement avro-based serializer #10890

Merged
merged 24 commits into from
Sep 3, 2020

Conversation

nguerrera
Copy link
Contributor

@nguerrera nguerrera commented Aug 27, 2020

Follow up on feedback after prior PR was merged.

First commit follows up on feedback in #10602 and reduces the interface count by hiding _response from the typing and inlining another base interface that wasn't useful on its own. The types in question are down to 3 as there is one kind of input and two kinds of outputs.

(There's a trick that can be played of shifting the content parameter into the return value to reduce to single output interface, but I still don't see any purpose in that, and I think it just confuses things by making it less clear what the service call actually expects and returns. I'm still willing to play this trick if anyone feels strongly about it, though at this point, I'd wait until after preview 1.)

Avro-based serializer that stores schema in schema registry

The rest is #10739. I have some open design questions flagged with //REVIEW in the PR still, but I believe that these can be resolved after Preview 1 unless there's strong feedback otherwise here. I've filed #10950 for October with a summary of what is also in these comments. I am also tracking making a final call on whether to collapse types further there.

Still needed for preview release

  • Add content to README
  • Add samples
  • Set up nightly tests (synced with Daniel on this today)

Please let me know if there's anything else that would be release blocking that I've missed.

Fixes #10739

@nguerrera
Copy link
Contributor Author

@xirzec @ramya-rao-a @bterlson Marking this ready for review.

I still have the same open design questions, but did not hear anything back on them, and think they could be adressed/finalized after the first preview so I filed #10950 to look at these in the next milestone. I've updated the PR description accordingly as well.

Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Nice work! I like a lot of things about this PR, including making a registry interface.

Besides my standard feedback, I tried to pay special attention to the comments asking for review feedback. I've seen folks before comment on their own PRs to get specific feedback, but this was a neat way to do it in-line.

Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Looks good! I'm excited to see more of Schema Registry in the future.

Copy link
Contributor

@willmtemple willmtemple left a comment

Choose a reason for hiding this comment

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

Tweak to the regex in dev-tool LGTM

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.

[Schema Registry] Add Avro Serializer that talks to Schema Registry
4 participants