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

proposal: create AdminClient interface #1048

Closed
buyology opened this issue Feb 13, 2018 · 5 comments
Closed

proposal: create AdminClient interface #1048

buyology opened this issue Feb 13, 2018 · 5 comments

Comments

@buyology
Copy link

buyology commented Feb 13, 2018

Now that we have almost all of the administrative messages in place (and one in the pipeline #1027) and the updated Metadata-message for finding the coordinator as a PR as necessitated by some requests (#1047) — a natural next step would be to provide some higher level interfaces (as requested in e.g. #1039).

I therefore propose a new AdminClient interface, with a draft looking something like below. It could potentially be split up into smaller units if that makes sense for increased composability.

type AdminClient interface {
	CreateTopic(topic string, detail *TopicDetail) error
	DeleteTopic(topic string) error
	CreatePartitions(topic string, count int32, assignment [][]int32, validateOnly bool) error
	DeleteRecords(topic string, partitionOffsets map[int32]int64) error

	DescribeConfig(resource ConfigResource) ([]ConfigEntry, error)
	AlterConfig(resourceType ConfigResourceType, name string, entries map[string]*string, validateOnly bool) error

	CreateAcl(resource Resource, acl Acl) error
	ListAcls(filter AclFilter) ([]ResourceAcls, error)
	DeleteAcl(filter AclFilter, validateOnly bool) ([]MatchingAcl, error)
}

cc @Mongey @mimaison @ongardie-ebay @chandradeepak

@eapache
Copy link
Contributor

eapache commented Feb 13, 2018

Something like this makes a lot of sense to me, 👍.

My question is around how this interacts with Client; does it build on top (like the producer/consumer) or does it stand alone? How does it interact with the client's cache (e.g. if you use the admin client to delete a topic, does that get busted from the underlying client's cache somehow)?

@eapache
Copy link
Contributor

eapache commented Feb 13, 2018

Having just read what I wrote here, I'm also wondering if we can stay away from Client in the naming just to avoid ambiguity. ClusterAdmin or BrokerAdmin or something?

@mimaison
Copy link
Contributor

Thanks for coming up with a proposal. It looks good overall. Just a few comments:

  • we also need an API to list/describe topics
  • many of the Kafka APIs can accept a list of topics/resources so it would be nice to be able to support this. For example create/delete several topics in 1 call
  • we probably want to return tuples like (*TopicError, error) to differentiate failing to send the request and Kafka errors.

@buyology
Copy link
Author

buyology commented Feb 13, 2018

My question is around how this interacts with Client; does it build on top (like the producer/consumer) or does it stand alone? How does it interact with the client's cache (e.g. if you use the admin client to delete a topic, does that get busted from the underlying client's cache somehow)?
I'm also wondering if we can stay away from Client in the naming just to avoid ambiguity. ClusterAdmin or BrokerAdmin or something?

@eapache — I agree with the notion that we probably want to draw a clear line of separation from the Client. Otherwise we might get obliged to do (potentially complex?) book keeping. Also, I guess most are willing to accept that since AFAIK other administrative endpoints have no direct interactions with the producing/consuming clients (i.e. if you use the Java equivalent).

As a consequence I guess we should not even provide a NewXXXFromClient(Client), but only a NewXXX(addrs, Config)?

+1 on the ClusterAdmin naming.

  • We also need an API to list/describe topics

@mimaison — agree that would be nice for symmetry.

Looking at the Java versions both ListTopics and DescribeTopics just use the MetadataRequest under the hood. Since there already is a "list" method (Topics() and Partitions(topic)) on the Client — maybe we should make this just return the detailed results — a DescribeTopics()?

  • many of the Kafka APIs can accept a list of topics/resources so it would be nice to be able to support this. For example create/delete several topics in 1 call
  • we probably want to return tuples like (*TopicError, error) to differentiate failing to send the request and Kafka errors.

Both very good points. And I would say that the (complexity of the) error return is very much related to whether you touch one or more resources simultaneously — if it's singular we could just return one error and the signature becomes pretty straightforward — if it's more than one we need to do the error separation and the input signature also becomes slightly more involved.

My thinking with principally suggesting singular was that if you e.g. want to create multiple topics in a go — then you are not far off from using the raw messages (the only thing you need to recognize is that you need to direct some of the operations to the coordinator).

Would it be too much to have both CreateTopic and CreateTopics?

@chandradeepak
Copy link
Contributor

@eapache , my few cents, I would go with BrokerManager for naming convention since it manages brokers in terms of topic creation , deletion etc. Also i am willing to contribute to this PR @buyology @eapache . Please let me know which would you want to split . I am happy to do it .

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

4 participants