Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[SchemaRegistry] add lru cache to avro serializer #20813
[SchemaRegistry] add lru cache to avro serializer #20813
Changes from 22 commits
7c48c74
3f47b00
a37f82e
eb5f749
8bfc931
62c1335
4bd0e5d
0bcf433
1d4daa1
00e602d
7ad7dc8
582f2b8
a76497f
2dd27be
942cddf
d8be661
5a9db19
18ab332
7453723
e3abf91
2b9baeb
0bf25b6
461ea57
34f4e38
52ec26b
52d87cf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
why cast
cached_schema
to be type of string here?I'm thinking of the accepted type of input
schema
here based on the discussion we just had today -- hide apache avro from the surfaces areas.maybe we could start by just supporting "str" first, because accepting bytes brings the problem of decoding bytes into string.
do we know what type fastavro is expecting for the schema, string, bytes or dict?
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.
lru_cache
does not work when the parameters are mutable (i.e.,avro.schema.Schema
), and we castschema
into a string in_get_schema-id
anyway and don't every use the actualSchema
object for anything.I think removing
Schema
as an input type is a good idea (in the separate PR for removing avro leaks), but just wondering if this is part of the surface? since thisserialize
method only acceptsstr/bytes
and it's only internally that we convert/pass aroundavro.schema.Schema
.fastavro
expects dicts for schema. (sample code here: [SchemaRegistry] investigate Apache Avro dependency #20708 (comment))str
for now instead ofbytes
+str
?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.
only support
str
as this aligns with other languages. This will already be addressed in this PR: https://github.com/Azure/azure-sdk-for-python/pull/20763/files.Merge this PR after.
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.
keeping everything, except for
bytes
as a type forschema
. that will be addressed in another PR which will be merged first. #20763I can deal with removing the
avro.schema.Schema
in the other PR for addressing types being leaked.