Gfodor/add throttle time to fetch response

[gfodor]

The Kafka protocol specifies that the v1 format of FetchResponse (introduced in 0.9.0) includes a throttle time field that precedes the message set. The current client seems to break against 0.9.0 if this field is not read properly. This patch extends the FetchResponse.Parser to be parameterized against the version of the protocol it needs to read, and then reads this field properly if the original request was in version 1 or higher.

(Note that the protocol documentation found here has a bug and mis-states the throttle time follows the message set -- it actually appears before the message set. The primary protocol docs properly describes the response.)

[msftclas]

Hi @gfodor, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

[gfodor]

CLA signed

[msftclas]

@gfodor, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

[gfodor]

One thing to note on this patch is that currently FetchRequest.CurrentVersion is currently set to 1 (which dictates the versionId of all FetchRequests), while the existing code (pre-patch) parses version 0 messages. So unfortunately this mismatch means that just dropping this patch in as-is will break anyone connecting to Kafka < 0.9.0 since the parser will now be expecting proper v1 messages since that is (and was) the default versionId. Two potential resolutions:

• Default the version to 0 (optionally removing the FetchRequest.CurrentVersion field.) Add versionId to the FetchRequest constructor and its callers (Consumer.Fetch, etc) for those who want to opt-in to make requests with the new protocol who are on Kafka >=0.9.0. This will ensure old code continues to work but will cause anyone on Kafka >= 0.9.0 to need to pass in the proper versionId to get the throttle time field filled in.
• Leave the default/current versionId to 1, but introduce the parameterization of versionId into the constructor/callers as above. Old clients will need to update their call sites to use version 0 explicitly if they are using Kafka < 0.9.0. One question here will be if to introduce the new constructor/call sites as overloads or not -- if they are overloads this will mean old code will still compile but will unexpectedly break on Kafka < 0.9.0, whereas adding to the method signatures will force users to specify their version as part of upgrading their KafkaNet DLL.

[riamandii]

Hi Greg,
Are you planning to add the changes for not breaking the existing code?

I would suggest resolution #1 to not break existing consumers/producers. What do you think?

Thanks!
Razvan

[gfodor]

Hey Razvan, yes I can make these changes. I'll have an updated PR in the next couple of days.

[gfodor]

See #44

[soumyajit-sahu]

Hi Greg! Thanks for tracking this down. I am going to make a change to set the version Id to 0. That will take care of keeping the lib compatible with all Kafka versions.
For the breaking change (versionId>0), we could have a new branch (say 0.9) for now. The lib needs some work to get the new 0.9 features anyways. What do you think?

[gfodor]

I have a PR over at #44 that adds support for both 0.9.0 and 0.10.0 protocol changes. It does so without any breaking changes by leaving the defaults to speak < 0.9.0 protocols, but requires the caller to specify the server protocol versions of the various messages if they want to speak >= 0.9.0 protocols. (Soon the client will be able to negotiate this, see https://cwiki.apache.org/confluence/display/KAFKA/KIP-35+-+Retrieving+protocol+version, due for 0.10.0, but that obviously deserves it's own patch.)

I can break that PR into two PRs to make this easier, the 0.9.0 changes are relatively basic whereas the 0.10.0 changes are more involved, not released yet, and probably require a more extended review. Does that make sense?