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 offset_manager for Kafka 0.9.0.0 #585

Merged
merged 1 commit into from
Dec 13, 2015

Conversation

horkhe
Copy link
Contributor

@horkhe horkhe commented Dec 12, 2015

Here is why the solution works.

if _, err := offsetManager.ManagePartition("does_not_exist", 123); err != ErrUnknownTopicOrPartition {
t.Fatal("Expected ErrUnknownTopicOrPartition when starting a partition offset manager for a partition that does not exist, got:", err)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this have to be removed? Is it because the new group protocol is abstract, and potentially could work for other sources besides kafka partitions? (e.g. Kafka Connect). Or is there another reason?

Copy link
Contributor

Choose a reason for hiding this comment

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

You no longer get back ErrUnknownTopicOrPartition, you get back ErrNoError with an offset of -1 (just like you do for topic/partitions that exist but haven't yet had an offset committed). I'm honestly not sure if this is a kafka bug, or an intended behaviour change.

Copy link
Contributor

Choose a reason for hiding this comment

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

@edenhill ^ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, it is just not an error anymore. Whether it is a bug or behaviour change we cannot think of anything smarter than stop checking that.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Given the differences in behavior between Kafka versions, removing this assertion is probably best.

@wvanbergen
Copy link
Contributor

Nice find 👍

@eapache
Copy link
Contributor

eapache commented Dec 12, 2015

Very nice find on setting the generation to -1.

@horkhe
Copy link
Contributor Author

horkhe commented Dec 12, 2015

I am having second thoughts about using -1 directly. I should probably define a constant e.g. groupGenerationUndefined. Thoughts guys?

@wvanbergen
Copy link
Contributor

👍 for groupGenerationUndefined as constant. Other that than I am happy to merge this.

@horkhe
Copy link
Contributor Author

horkhe commented Dec 13, 2015

Added the constant and squashed.

wvanbergen added a commit that referenced this pull request Dec 13, 2015
Fix offset_manager for Kafka 0.9.0.0
@wvanbergen wvanbergen merged commit 1fdcdc2 into IBM:master Dec 13, 2015
@horkhe horkhe deleted the maxim/offset_manager branch December 14, 2015 00:16
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