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 client.Isr to determine in-sync replicas #872

Merged
merged 3 commits into from May 1, 2017
Merged

Add client.Isr to determine in-sync replicas #872

merged 3 commits into from May 1, 2017

Conversation

pd
Copy link
Contributor

@pd pd commented May 1, 2017

This restores the commit from @funkygao in PR #566, with an additional commit to expose the function on the Client interface and add some basic tests around its functionality.

The PR was previously closed with the suggestion that Zookeeper should be used directly, because, at the time, it was possible to receive stale information from Kafka. If I'm reading the linked issue correctly, that issue has been resolved since Kafka 0.9, so I'd like this patch to be reconsidered now.

I personally want this so that I can then add ISR support to the excellent kt, which is the workhorse of a fair amount of the Kafka automation tooling I use.

Copy link
Contributor

@eapache eapache left a comment

Choose a reason for hiding this comment

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

If I'm reading the linked issue correctly, that issue has been resolved since Kafka 0.9

Hmm, that's also how I'm reading the linked issue, but I'm a bit suspicious still because I thought I had been following this issue more closely and that there were further complications. Perhaps I'm just being paranoid though, I can't find any of the problems I seem to remember.

client.go Outdated
@@ -38,6 +38,9 @@ type Client interface {
// Replicas returns the set of all replica IDs for the given partition.
Replicas(topic string, partitionID int32) ([]int32, error)

// Isr returns the set of in-sync replica IDs for the given partition.
Isr(topic string, partitionID int32) ([]int32, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

For a public API I would prefer this be a bit more explicit: ReplicasInSync or InSyncReplicas or something like that. It would also be nice if the docs explained (or linked to an explanation) of what it means to be in sync.

@pd
Copy link
Contributor Author

pd commented May 1, 2017

@eapache I've renamed it to InSyncReplicas, and added some semblance of an explanation of what that means to the godoc. I wasn't able to find any particularly useful, official documentation of what ISR actually means, aside from some things on cwiki that haven't been edited since 2013.

Improved documentation suggestions welcome. =)

@eapache
Copy link
Contributor

eapache commented May 1, 2017

This is great, thanks!

@eapache eapache merged commit bdb312e into IBM:master May 1, 2017
@pd pd deleted the isrs branch May 1, 2017 20:02
pd added a commit to pd/kt that referenced this pull request May 1, 2017
To get access to the `client.InSyncReplicas` function, added here:

IBM/sarama#872
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

3 participants