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

Fix OffsetCommitRequest #390

Merged
merged 1 commit into from
Mar 24, 2015
Merged

Fix OffsetCommitRequest #390

merged 1 commit into from
Mar 24, 2015

Conversation

eapache
Copy link
Contributor

@eapache eapache commented Mar 24, 2015

Apparently the kafka protocol spec has been inconsistent for a while,
documenting v1 of this message but claiming it was still v0. Jun finally updated
it, so now we can properly implement both versions correctly.

@Shopify/kafka

Apparently the kafka protocol spec has been inconsistent for a while,
documenting v1 of this message but claiming it was still v0. Jun finally updated
it, so now we can properly implement *both* versions correctly.
@wvanbergen
Copy link
Contributor

Do we have to send the right version of the message to the right Kafka version? Or can kafka 0.8.2 handle the old message as well? Given that offset management doesn't really work in 0.8.1 I have no toruble always using the new version, or at least make that the default.

@eapache
Copy link
Contributor Author

eapache commented Mar 24, 2015

AFAIK 0.8.2 can handle both, otherwise you wouldn't necessarily be able to upgrade without downtime.

@eapache
Copy link
Contributor Author

eapache commented Mar 24, 2015

The spec now says "v0 (supported in 0.8.1 or later)"

@wvanbergen
Copy link
Contributor

Yeah; I think Kafka 0.8.1 can read the message, but it doesn't do anything useful with it. The functional test for offset management was failing pretty hard on kafka 0.8.1.1 when using the v0 message format. So I don't see it as very useful to keep the v0 format around.

@eapache
Copy link
Contributor Author

eapache commented Mar 24, 2015

We weren't sending the v0 format before, we were sending v1 and saying v0 because the wiki was wrong :(

@wvanbergen
Copy link
Contributor

Aha OK. 👍 in that case

eapache added a commit that referenced this pull request Mar 24, 2015
@eapache eapache merged commit eb30a57 into master Mar 24, 2015
@eapache eapache deleted the offset-commit-request branch March 24, 2015 14:43
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.

None yet

2 participants