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

Add support for Kafka 0.11 Record and RecordBatch #969

Closed
wants to merge 1 commit into from
Closed

Add support for Kafka 0.11 Record and RecordBatch #969

wants to merge 1 commit into from

Conversation

wladh
Copy link
Contributor

@wladh wladh commented Oct 18, 2017

The new records format (including headers) is supported, however there's
no support for transactions or idempotent messages.
This is the first step towards issue #901

The new records format (including headers) is supported, however there's
no support for transactions or idempotent messages.
@eapache
Copy link
Contributor

eapache commented Oct 19, 2017

Thanks so much for this contribution, it looks pretty good. Unfortunately it's an enormous PR (1600 lines!) and I'm having trouble untangling the pieces to review it properly.

Could you split it up and submit it in a couple of smaller PRs please? Off the top of my head there are at least the following chunks that are separable:

  • varint support in the encoder layer
  • Castagnoli support for CRCs
  • implementation of the new record and recordbatch structs at the protocol layer
  • support for record/recordbatch in the producer
  • support for record/recordbatch in the consumer

Don't be afraid to split it up even further though if you find good divisions. The time it takes me to review something is usually about quadratic on the number of lines of code, so the smaller the better (within reason).

@wladh
Copy link
Contributor Author

wladh commented Oct 19, 2017

Sure, I prefer small commits myself.

@wladh wladh closed this Oct 24, 2017
@wladh
Copy link
Contributor Author

wladh commented Oct 24, 2017

Do you want multiple PRs or is multiple commits in a single PR fine?

@eapache
Copy link
Contributor

eapache commented Oct 24, 2017

Multiple PRs would be preferable, but I can deal with either

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