-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
PIP-85 Add Schema Information to Message in Java Client API #10476
PIP-85 Add Schema Information to Message in Java Client API #10476
Conversation
This PR is a "draft", it is a reference for the discussion on the GDoc, please read the GDoc before commenting here |
b35fc08
to
956e786
Compare
Latest commit broke the tests. |
148c6ea
to
b585bb2
Compare
The patch is ready for review. |
d635506
to
49ddab9
Compare
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.
LGTM! left some comment.
if (schema == null) { | ||
return Optional.empty(); | ||
} | ||
if (schema instanceof AutoConsumeSchema) { |
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 AutoConsumeSchema
cant extend AbstractSchema
?
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.
@congbobo184 I am afraid that this change may have side effects, AutoConsumeSchema
is very special as well as AbstractSchema
If you want I can try to apply this suggestion in a follow up patch
* @throws SchemaSerializationException in case of unknown schema version | ||
* @throws NullPointerException in case of null schemaVersion | ||
*/ | ||
public Schema<?> atSchemaVersion(byte[] schemaVersion) throws SchemaSerializationException { |
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 named atSchemaVersion
? may getSchemaByVersion
is easy to understand?
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 sounds to me pretty good, let's see this example:
Schema<?> schema = .....
Schema<?> schemaVersion2 = schema.atSchemaVersion(2);
In this example is like having immutable objects, and you call a method to create a new object with different content
With getSchemaByVersion it looks to that a Schema is a "container" (or a registry) for Schemas
Schema<?> schema = .....
Schema<?> schemaVersion2 = schema.getSchemaByVersion(2);
…0476) This is the implementation for PIP-85 https://docs.google.com/document/d/1VWi5LHP44V31nP4bCui9d5RXwH6xc_phrUes6tvNguk/ - introduce `Optional<Schema<?>> Message.getReaderSchema()` - new Public API - introduce `Optional<Object>` SchemaReader.getNativeSchema()` - new Public API - introduce `Schema<?> atSchemaVersion(schemaVersion)` to `AbstractSchema` (private API) - make `KeyValueSchema` extend AbstractSchema (private API) - rename MessageImpl and TopicMessageImpl `getSchema` to `getSchemaInternal` The schema returned by Message.getReaderSchema has these properties: - getSchemaInfo returns the actual schema used to decode the Message - getNativeSchema returns the original native (ie. Avro) Schema for the correct version - it can be used for "decoding" payloads - it cannot be used for "encoding" payloads (cherry picked from commit 4224110)
This is the implementation for PIP-85
https://docs.google.com/document/d/1VWi5LHP44V31nP4bCui9d5RXwH6xc_phrUes6tvNguk/
Optional<Schema<?>> Message.getReaderSchema()
- new Public APIOptional<Object>
SchemaReader.getNativeSchema()` - new Public APISchema<?> atSchemaVersion(schemaVersion)
toAbstractSchema
(private API)KeyValueSchema
extend AbstractSchema (private API)getSchema
togetSchemaInternal
The schema returned by Message.getReaderSchema has these properties: