-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Conversation
sdk/schemaregistry/schema-registry-avro-serializer/src/schemaRegistryAvroSerializer.ts
Outdated
Show resolved
Hide resolved
sdk/schemaregistry/schema-registry-avro-serializer/src/schemaRegistryAvroSerializer.ts
Outdated
Show resolved
Hide resolved
@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. |
There was a problem hiding this 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.
sdk/schemaregistry/schema-registry-avro/test/schemaRegistryAvroSerializer.spec.ts
Outdated
Show resolved
Hide resolved
sdk/schemaregistry/schema-registry-avro/test/schemaRegistryAvroSerializer.spec.ts
Show resolved
Hide resolved
sdk/schemaregistry/schema-registry-avro/test/schemaRegistryAvroSerializer.spec.ts
Show resolved
Hide resolved
Co-authored-by: Jeff Fisher <xirzec@xirzec.com>
There was a problem hiding this 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.
There was a problem hiding this 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
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
Please let me know if there's anything else that would be release blocking that I've missed.
Fixes #10739