Skip to content

Conversation

@ewencp
Copy link
Contributor

@ewencp ewencp commented Feb 2, 2015

This provides the producer side of the Avro patch. This turned out to be tricky to get right because of a number of semi-conflicting goals. Getting reuse between the input entity classes, including for both producing to topcis and partitions, using a single set of producer code that supports all the formats, handling the two formats required to support Avro (the original JSON input + GenericRecord and translating between them at the right time), and avoiding complete duplication of test code was difficult to balance. Most of the more unusual looking classes (e.g. SchemaHolder, ProduceRecordBase) are due to trying to balance this.

I think the main implementation eventually ended up working out OK even if it is a bit more convoluted than I would have liked. I am less happy with the tests, where there is more duplication of code than I would like. I think we could probably get more reuse, but I was also concerned with that code becoming too confusing if it used too many generic helper methods that could be shared between the binary and Avro producers. Suggestions for how to improve that code would be welcome.

This includes some substantial refactoring in order to support multiple formats
with different producer types, API endpoints that use different content types to
target different producers with different serializers, internal support for
translating the raw JSON data that is initially decoded into Avro GenericRecords
that can be used with the Avro serializer, and support for passing schemas into
requests and returning schema ID information with responses.
@ewencp
Copy link
Contributor Author

ewencp commented Feb 2, 2015

@nehanarkhede Do you want to review this? The reasoning behind some of the design is sometimes subtle -- only discovered after trying to implement it other, simpler ways. Tried to leave notes where the design isn't necessarily intuitive.

This patch is pretty big mostly because handling different input formats and the translation from JSON -> GenericRecord turned out to be kind of tricky. I think the consumer side should be simpler.

@ewencp
Copy link
Contributor Author

ewencp commented Feb 3, 2015

Blocked by confluentinc/schema-registry#94

Choose a reason for hiding this comment

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

Seems like this API is unused. Do you have an expected usage for this API in a future patch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I think I lost one of my changes during rebase. Fixed (including fix to AvroProducerTest to verify the offsets this is used to set).

ewencp added a commit that referenced this pull request Feb 4, 2015
@ewencp ewencp merged commit d3bb3c1 into master Feb 4, 2015
@ewencp ewencp deleted the avro-producer branch February 4, 2015 02:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants