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

Rename to SchemaRegistryApacheAvroSerializer. Group name is optional. #24622

Merged
merged 11 commits into from
Oct 7, 2021

Conversation

conniey
Copy link
Member

@conniey conniey commented Oct 7, 2021

  • Rename SchemaRegistryAvroSerializer to SchemaRegistryApacheAvroSerializer.
  • Rename schemaFormat -> format to align with other languages.
  • Make groupName optional as per Arch board review.
    • It's not required for deserialization because the schemaId would be in the payload (and it is globally unique).
  • Add package-private options bag for creating Serializer so it doesn't become bloated with constructor parameters.
    • Adding options.getMaxCacheSize() in preparation for November's LRU cache options.

Related #24221

@conniey conniey self-assigned this Oct 7, 2021
@conniey conniey requested review from sjkwak and srnagar as code owners October 7, 2021 17:20
@conniey conniey changed the title Update cache Rename to SchemaRegistryApacheAvroSerializer. Group name is optional. Oct 7, 2021
@azure-sdk
Copy link
Collaborator

API changes have been detected in com.azure:azure-data-schemaregistry. You can review API changes here

@azure-sdk
Copy link
Collaborator

API changes have been detected in com.azure:azure-data-schemaregistry-avro. You can review API changes here

* @param maxCacheSize The maximum cache size for the serializer.
*/
SerializerOptions(String schemaGroup, boolean autoRegisterSchemas, int maxCacheSize) {
this.schemaGroup = schemaGroup;
Copy link
Member

Choose a reason for hiding this comment

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

Can this be null? Previously, this was a required field and we had a null check in the serializer constructor. If this can't be null, let's add a null check here.

Copy link
Member Author

Choose a reason for hiding this comment

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

It can be null now. From the arch board review, if you're only using it for deserialization, the stream contains the ID which is globally unique, so you can just get by an id without the schema group.

@@ -26,26 +26,31 @@
/**
* Schema Registry-based serializer implementation for Avro data format.
Copy link
Member

Choose a reason for hiding this comment

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

Update the javadoc as well to specify that this is Apache Avro data format.

*
* @return The maximum cache size.
*/
int getMaxCacheSize() {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be long? I don't expect the cache size to be beyond max int but do we want to have the flexibility?

Copy link
Member Author

Choose a reason for hiding this comment

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

For GA, we are not exposing this max cache size to be configurable; so in November, it'll just be 128. It won't be a breaking change if we decide after GA to expose the cache options.

Comment on lines 117 to 119
} else if (Objects.isNull(context)) {
return FluxUtil.monoError(logger, new NullPointerException("'context' should not be null."));
}
Copy link
Member

Choose a reason for hiding this comment

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

Context can be null and if it is, we should use Context.NONE and not throw.

conniey and others added 3 commits October 7, 2021 15:02
…a/com/azure/data/schemaregistry/avro/SerializerOptions.java

Co-authored-by: Srikanta <51379715+srnagar@users.noreply.github.com>
Copy link
Member

@srnagar srnagar left a comment

Choose a reason for hiding this comment

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

LGTM!

@conniey conniey merged commit 94fba96 into Azure:main Oct 7, 2021
@conniey conniey deleted the update-cache branch October 7, 2021 23:50
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.

3 participants