-
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
Conversation
API changes have been detected in |
API changes have been detected in |
9a00ddd
to
6091b10
Compare
@@ -10,7 +10,7 @@ | |||
* An abstraction for a message containing a content type along with its data. | |||
*/ | |||
@Fluent | |||
public class MessageWithMetadata { | |||
public class BinaryContent { |
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
and BinaryContent
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.
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.
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 use BinaryData
and not BinaryContent
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.
@@ -10,7 +10,7 @@ | |||
* An abstraction for a message containing a content type along with its data. | |||
*/ | |||
@Fluent | |||
public class MessageWithMetadata { | |||
public class BinaryContent { |
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
and BinaryContent
are very similar.
sdk/schemaregistry/azure-data-schemaregistry-apacheavro/README.md
Outdated
Show resolved
Hide resolved
...java/com/azure/data/schemaregistry/apacheavro/SchemaRegistryApacheAvroSerializerBuilder.java
Outdated
Show resolved
Hide resolved
...stry-apacheavro/src/samples/java/com/azure/data/schemaregistry/apacheavro/ReadmeSamples.java
Outdated
Show resolved
Hide resolved
API changes have been detected in |
Description
Based on conversations with architects, this updates:
PRs from other languages:
If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines