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
Passing schema with id to TypeInfo and ConfluentKafkaAvroDeserializer #1305
Conversation
…or further optimizations
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.
Good change, let's look on my comments.
} | ||
|
||
object RuntimeSchemaData { | ||
def apply(schema: Schema, idOpt: Option[Int]): RuntimeSchemaData = |
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.
consistently schemaIdOpt
?
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.
fixed
* It is lightened version of Confluent's SchemaMetadata. We don't want to use their class, because our SchemaRegistryClient | ||
* is not coupled with concrete schema registry implementation. | ||
*/ | ||
case class SchemaWithMetadata(schema: Schema, id: Int) |
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.
- If we have
RuntimeSchemaData
then maybe better name will be justSchemaData
? - Hmm, I think about add there version: Option[Int]?
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.
Ad 1. I want to has similar naming as Confluent does. IMO it is no need to make it looks similar like RuntimeSchemaData
because there are for other purposes.
Ad 2. For now YAGNI, there is many other fields that we can have here, but IMO we shouldn't put all because we don't know how looks like other schema registry implementations. Take a look on full confluent's SchemaMetada
:
private int id;
private int version;
private String schemaType;
private String schema;
private List<SchemaReference> references;
val buffer = ConfluentUtils.parsePayloadToByteBuffer(payload).valueOr(ex => throw ex) | ||
read(buffer, readerSchema) | ||
} | ||
|
||
protected def read(buffer: ByteBuffer, expectedSchema: Schema): AnyRef = { | ||
protected def read(buffer: ByteBuffer, expectedSchema: RuntimeSchemaData): AnyRef = { |
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.
readerSchema -> readerSchemaData
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.
Fixed in all places. You don't need to add comments in all places ;)
… for further optimizations