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.

Sign up for GitHub

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

Jump to bottom

[Issue #6842] feat(schema): allow for schema reader and writer registration on SchemaDefinition #6905

Merged
merged 5 commits into from
May 18, 2020
Merged

[Issue #6842] feat(schema): allow for schema reader and writer registration on SchemaDefinition #6905

merged 5 commits into from
May 18, 2020

Conversation

Persi
Copy link
Contributor

@Persi Persi commented May 7, 2020 •
edited
Loading

Fixes #6842

Motivation and Modifications

With this feature it is possible to configure own SchemaReader and SchemaWriter on SchemaDefinition instantiation. With this it is possible to e.g. reuse already configured Jackson ObjectMapper instances.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API: (yes / no)
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)
  • The rest endpoints: (yes / no)
  • The admin cli options: (yes / no)
  • Anything that affects deployment: (yes / no / don't know)

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@Persi
feat(schema): allow for schema reader and writer registration on Sche…
5af0792 
…maDefinition

With this feature it is possible to configure own SchemaReader and SchemaWriter on SchemaDefinition instantiation. With this it is possible to e.g. reuse already configured Jackson ObjectMapper instances.
hansenc
hansenc reviewed May 7, 2020
View reviewed changes
pulsar-client-api/src/main/java/org/apache/pulsar/client/api/schema/SchemaDefinition.java
*
* @return optional containing configured schema reader or empty optional if none is configure
*/
Optional<SchemaReader<T>> getSchemaReaderOpt();
Copy link

Choose a reason for hiding this comment

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

This interface is used for more than just JSON schemas, right? If so, it will be confusing for users to be able to set a reader and writer for any type of schema when they will only be used for JSON schemas and ignored otherwise. So we either need to support this broadly, or instead we can overload JSONSchema.of() for a more targeted approach.

Copy link
Contributor Author

@Persi Persi May 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This interface can be used for other schemas as well, but both SchemaReader and SchemaWriter as well. I see what you mean, did not recognize the Producer and Consumer instantiators with Schema as parameter. But I am not sure if Schema class is meant to be used like that from outside the framework. As they already introduced some default Schema instantiators in the client api Schema class, I would not promote Schema.of for public use, as it is an internal of the framework.

Copy link

@hansenc hansenc May 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, you'll probably want to overload Schema.JSON(Class<T>) and Schema.JSON(SchemaDefinition) as well as JSONSchema.of(). All of these are referenced in the docs so presumably they are meant for public use. (Though I would have guessed JSONSchema.of is not meant for public use since it's in the impl package. 🤷)

@Persi
Copy link
Contributor Author

Persi commented May 7, 2020

@hansenc Thank you for your review and feedback!

sijie
sijie reviewed May 8, 2020
View reviewed changes
Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

The change looks great! Can you add unit tests for validating that the provided schema reader and writer are actually used for serialization and deserialization?

@sijie sijie added this to the 2.6.0 milestone May 8, 2020
@Persi
Copy link
Contributor Author

Persi commented May 8, 2020

The change looks great! Can you add unit tests for validating that the provided schema reader and writer are actually used for serialization and deserialization?

Thank your for your review! I will add some tests soon.

@Persi
fix: SchemaDefinitionBuilderImpl build verification
ed8cc7b 
Added unit and integration tests for SchemaReader and SchemaWriter registration
codelipenghui
codelipenghui reviewed May 12, 2020
View reviewed changes
Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

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

The change looks good. I just left a minor comment.

pulsar-broker/src/test/java/org/apache/pulsar/client/impl/BrokerClientIntegrationTest.java Outdated
import org.apache.pulsar.client.api.PulsarClient;
import org.apache.pulsar.client.api.PulsarClientException;
import org.apache.pulsar.client.api.SubscriptionType;
import org.apache.pulsar.client.api.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid using import .*

@sijie sijie merged commit 8b12635 into apache:master May 18, 2020
@sijie sijie mentioned this pull request May 20, 2020
Closed
Huanli-Meng pushed a commit to Huanli-Meng/pulsar that referenced this pull request May 27, 2020
@Persi @Huanli-Meng
[Issue apache#6842] feat(schema): allow for schema reader and writer …
6297e4d 
…registration on SchemaDefinition (apache#6905)

Fixes apache#6842 

### Motivation and Modifications

With this feature it is possible to configure own SchemaReader and SchemaWriter on SchemaDefinition instantiation. With this it is possible to e.g. reuse already configured Jackson ObjectMapper instances.
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Aug 24, 2020
@Persi @huangdx0726
[Issue apache#6842] feat(schema): allow for schema reader and writer …
8cd3129 
…registration on SchemaDefinition (apache#6905)

Fixes apache#6842 

### Motivation and Modifications

With this feature it is possible to configure own SchemaReader and SchemaWriter on SchemaDefinition instantiation. With this it is possible to e.g. reuse already configured Jackson ObjectMapper instances.
@tomas-c tomas-c mentioned this pull request Sep 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hansenc hansenc hansenc left review comments

@codelipenghui codelipenghui codelipenghui left review comments

@sijie sijie sijie approved these changes

@jiazhai jiazhai Awaiting requested review from jiazhai

Assignees

@Persi Persi

Labels
None yet
Projects
None yet
Milestone
2.6.0
Development

Successfully merging this pull request may close these issues.

Allow providing a self-created ObjectMapper instance for Jackson to be able to use Jackson extensions
4 participants
@Persi @hansenc @sijie @codelipenghui