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

Expose consistent client method set #23

Closed
eapache opened this issue Aug 22, 2013 · 2 comments
Closed

Expose consistent client method set #23

eapache opened this issue Aug 22, 2013 · 2 comments

Comments

@eapache
Copy link
Contributor

eapache commented Aug 22, 2013

The Client object has basically no public methods right now, it is just a place to dump methods (mostly broker and metadata tracking) that are shared by the Consumer and the Producer. Its primary functions should have specific semantics defined and then be published so other people can make use of them.

Probably depends on getting #15 right first...

I'm thinking (as a rough first draft):

  • Leader(topic, partition) returns the broker object leading the topic/partition
  • Topics() returns the list of topics for the cluster
  • Partitions(topic) returns the list of partitions for the topic
  • RefreshMetadata(topics[])

Open Questions:

  • Does it make sense to store/return other metadata (replicas or ISRs, for example)? Does any of this provide benefit beyond just sending a MetadataRequest using the low-level Broker object?
  • How are brokers returned to the client when no longer needed? (the current disconnectBroker feels hacky, this really depends on Better Broker Connection Management #15)
  • Do we want to provide RefreshTopic(topic) or RefreshAllTopics() helper functions or something like that?
  • Does it make sense to expose any()? It's only useful for metadata, so this probably depends if we expose all metadata or if a user might want to get it themselves.
eapache pushed a commit that referenced this issue Aug 26, 2013
The producer and consumer still make use of some private methods, but those are
harder to expose consistently. This is at least a good start towards #23.
eapache pushed a commit that referenced this issue Aug 27, 2013
Also add a more comprehensive comment to disconnectBroker().

This is the last piece of #23 that doesn't heavily depend on getting #15 right
first.
@eapache
Copy link
Contributor Author

eapache commented Nov 10, 2013

While poking around various implementations of #18 I finally figured out what I want to do with disconnectBroker (ie how to get rid of it). It really has two purposes:

  • it forces a metadata refresh to happen on the next client request for that broker (ie call to Leader or similar)
  • even if the metadata isn't stale, it bounces the broker connection so that (for example) a stale connection to the correct broker is put back in a good state

The metadata refresh we can already force with a call to one of the public Refresh methods. The connection bounce is interesting, because I originally added this comment to the client.update method: "[if the broker hasn't changed] ignore it, replacing our existing one would just bounce the connection". However if we make the metadata refresh safely bounce connections for any broker that isn't stale we could in principle replace all "public" calls to disconnectBroker with straight calls to RefreshTopicMetadata.

We still have one internal call to it that would need dealing with (perhaps it just gets folded into the any method?) and parts of it are still tangled up in #15 but I think I've made progress in at least understanding the problem space.

@eapache
Copy link
Contributor Author

eapache commented Feb 24, 2015

All of the methods I started this ticket for have been created (a while ago, in most cases). The only open question is broker disconnection, which is still better-tracked by #15. This can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant