-
Notifications
You must be signed in to change notification settings - Fork 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: Change to use BinaryContent and change Encoder to Serializer #27598
Closed
Closed
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
a18c81e
Rename MessageWithMetadata -> BinaryContent
conniey 3aa403d
Rename to BinaryContentTest
conniey 774ede8
Add unreleased_ entry
conniey 3624573
Update CHANGELOG.
conniey 76816bb
Add reference to unreleased in pom.xml
conniey a3c7d43
Fix build breaks from rename.
conniey bbe394e
Adding explicit dependency on azure-core.
conniey 0a23693
Updating class name from Encoder -> Serializer.
conniey 74d5e10
Rename encode/decode to deserialize/serialize.
conniey 394cee3
Update CHANgELOG
conniey f94f329
Rename playback files.
conniey 6091b10
Fix README file snippets.
conniey 60cd114
Fix CHANGELOG entries.
conniey 1771dc3
Fix version update tag.
conniey d89f6d3
Fixing README entries.
conniey 6b43d94
Fix broken link
conniey File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
We would now have BinaryData and BinaryContent as public types and another implementation type i.e. BinaryDataContent. This is likely going to confuse us in 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.
Yeah, this is going to be confusing.
BinaryData
andBinaryContent
are very similar.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.
This was based on architect feedback. The other languages also have moved their names to this and also have BinaryData. I can add you to the thread if you want.
https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/core/Azure.Core.Experimental/api/Azure.Core.Experimental.netstandard2.0.cs#L9
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.
The idea is that BinaryContent contains BinaryData along with the content type. We didn't think it would be confusing as they are two related and similarly named types, rather than being just two similarly named types.
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.
/cc @tg-msft
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.
@pallavit, @srnagar, could you please elaborate what's confusing here and what concrete implications of this confusion you see? There are many types in all platforms that are similar, yet they are not considered confusing, e.g. Int32 and Int64 are super similar.
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.
I set-up a quick chat on Monday. Might be easier to get all the thoughts in the same room.
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.
Java already has BinaryDataContent which is wrapped by
BinaryData
but that's currently internal. Also, when deciding which type to use, would there be a scenario where we want to useBinaryData
and notBinaryContent
given that content type in BinaryContent is optional?One option was to move content type property to BinaryData but this may not be possible as we want to make this the super type for Event Hubs and Service Bus event and message types. If this type is intended for extension in Event Hubs and Service Bus, should we make this type abstract?
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.
It was initially abstract, but we made it concrete to make it easier for use with Schema Registry that didn't involve Service Bus/Event Hubs. However, I think we can still make this scenario easy by having a private implementation that we return if the user doesn't specify a more specific BinaryContent type. This would allow us to move the type back to being abstract which I believe would address your concerns around any confusion that might arise from how this type is meant to be used.
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.
Here is what this change would look like in .NET - Azure/azure-sdk-for-net#27525
After making the class abstract, I needed to add two additional generic overloads that only took the TData instead of both the TEnvelope and TData. This was necessary in order to leave in our generic type constraint that TEnvelope have an empty public constructor. When using the new overloads we would construct the internal DefaultBinaryContent and return that to the user. The new overloads make the sample code simpler since now the generic arguments can be completely inferred based on the input data.