admin: Add some missing admin methods #1178

birdayz · 2018-09-27T23:40:14Z

This PR adds the following admin client methods:
DescribeTopic
DescribeConsumerGroup
ListConsumerGroups
ListConsumerGroupOffsets

For ListConsumerGroups, all brokers are queried (the java client does it the same way). Since this will be super slow with multiple brokers, i made an implementation with parallel requests.
The others methods are rather straight forward.
I'll be happy about comments on the API, is it fine?
I'm not 100% sure - for the groups i chose it to be 'singular' - each method call is for one consumer group. For topics it's a slice of topics as parameter - in my case (building a kafka cli tool) it's very often the case that multiple topics are requested, so i kept it this way.

In #1155 (another addition to the admin client) i wondered how the API should look like. I kept it simple here. and reused types from the response messages.

I also added mock tests for these methods.

birdayz · 2018-09-28T10:08:13Z

i renamed it to DescribeTopics, so it's consistent with its parameters and return value.

birdayz · 2018-10-14T07:32:05Z

@bai can you have a look?

jeffwidman · 2019-01-04T05:43:15Z

I'm just checking out Sarama for the first time, so not sure how closely you try to follow the Java reference clients. However, I added these same methods to kafka-python, so had one design comment based on having already played with the kafka protocol here:

A common scenario in monitoring situations is to fetch the offsets/calculate consumer lag for many/all consumer groups in the cluster.
ListConsumerGroupOffsets is a very chatty call because it will first lookup the group's coordinator. So it requires two network round trips.
However, when listing consumer groups, if a broker returns a consumer group, you know it's the coordinator.
If you allow ListConsumerGroups to have an optional param of specifying a single broker and ListConsumerGroupOffsets to have an optional param of specifying a known group coordinator than you can leverage this to nearly halve the number of network calls required to fetch offsets for all consumer groups:
1. Loop through all brokers, calling ListConsumerGroups against each one.
2. For the consumer groups returned, call ListConsumerGroupOffsets, using the group coordinator identified above.

Java doesn't currently allow these overrides, but they were useful enough that we chose to include them in kafka-python, even though in general we try to follow the Java clients very closely: https://kafka-python.readthedocs.io/en/master/apidoc/KafkaAdminClient.html#kafka.admin.KafkaAdminClient.list_consumer_group_offsets

You can see how I then leveraged this when patching the Datadog check here:
https://github.com/DataDog/integrations-core/pull/2730/files#diff-deed983f73a04522ac621d7f6d0c8403R248

Note that I'm a total n00b to Go, so when I didn't find these overrides, it may be simply my misreading of the code and they may already be there...

birdayz · 2019-01-20T21:57:16Z

@bai Sorry for being annoying, but how could we move this forward? Or is this feature not desirable in sarama?
@jeffwidman i'm happy to discuss the specifics of an implementation if this going anywhere, especially since some aspects of the java reference implementation of the admin client are rather unlucky, in my opinion (thus, it makes sense to do some things different)

varun06 · 2019-01-22T15:13:37Z

admin.go

+	ListConsumerGroups() (map[string]string, error)
+
+	// Describe some group IDs in the cluster.
+	DescribeConsumerGroup(group string) (*GroupDescription, error)


Can you please update the comment, it is not very clear here.

varun06 · 2019-01-22T15:24:39Z

admin.go

@@ -380,3 +417,87 @@ func (ca *clusterAdmin) DeleteACL(filter AclFilter, validateOnly bool) ([]Matchi
 	}
 	return mAcls, nil
 }
+
+func (ca *clusterAdmin) DescribeConsumerGroup(group string) (*GroupDescription, error) {


I wonder if it works better as DescribeConsumerGroups(groups []string), coz controller takes a slice of string anyway, and that way if you only want it for one group, you can only pass that too?

i agree. i now improved it to accept a slice of group names. in addition, the logic is now quite smart to first build a map of brokers -> []string groups, so each group coordinator is contacted only once. in a trivial test / cluster with just 8 groups this saves me a second (50% savings)..which will have much a bigger impact on larger clusters.

varun06 · 2019-01-22T15:25:55Z

admin.go

+		wg.Add(1)
+		go func(b *Broker, conf *Config) {
+			defer wg.Done()
+			_ = b.Open(conf) // Ensure that broker is opened


you can do

Suggested change

_ = b.Open(conf) // Ensure that broker is opened

b.Open(conf) // Ensure that broker is opened

i can, but then we don't document that there's an error (deliberately) ignored here.

in fact errcheck will fail travis if i do what you suggest.. https://api.travis-ci.org/v3/job/490310704/log.txt i reverted that.

Signed-off-by: Dominic Evans <dominic.evans@uk.ibm.com>

birdayz · 2019-02-07T23:40:46Z

@dnwe i added your commits of your PR #1155 ..since my work is based on that / related. since your PR is also pending and i require it as well, do you mind pushing this forward together? would be nice if we could get this finally merged :) it's becoming quite tedious to maintain my own fork for so long :(
if it's not OK, i can remove it from my PR.

birdayz · 2019-02-08T00:07:33Z

Ci failed with a strange error:

# cd .; git clone https://go.googlesource.com/tools /home/travis/gopath/src/golang.org/x/tools
Cloning into '/home/travis/gopath/src/golang.org/x/tools'...
fatal: remote error: Access denied to IP 35.188.1.99
package golang.org/x/tools/go/packages: exit status 128

how can i trigger a re-run?

DescribeCluster DescribeTopic DescribeConsumerGroups ListConsumerGroups ListConsumerGroupOffsets

varun06 · 2019-02-08T00:09:24Z

Try closing and reopen again.

…

Sent from my iPhone On Feb 7, 2019, at 6:07 PM, Johannes Brüderl <notifications@github.com<mailto:notifications@github.com>> wrote: Ci failed with a strange error: # cd .; git clone https://go.googlesource.com/tools /home/travis/gopath/src/golang.org/x/tools<http://golang.org/x/tools> Cloning into '/home/travis/gopath/src/golang.org/x/tools'<http://golang.org/x/tools'>... fatal: remote error: Access denied to IP 35.188.1.99 package golang.org/x/tools/go/packages:<http://golang.org/x/tools/go/packages:> exit status 128 how can i trigger a re-run? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub<#1178 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABOX5pJ-Iixx_tLtd1pN_jVGgewtp6WMks5vLL_HgaJpZM4W9n9p>.

birdayz · 2019-02-08T00:21:44Z

great. it's green now :)

bai · 2019-02-08T08:20:25Z

Thanks!

bai · 2019-02-08T08:22:21Z

This is lovely work 💯

bai requested a review from varun06 January 21, 2019 03:21

varun06 reviewed Jan 22, 2019

View reviewed changes

ghost added cla-needed and removed cla-needed labels Feb 7, 2019

dnwe added 3 commits February 8, 2019 00:24

Add ListTopics func to ClusterAdmin interface

dae6d49

Signed-off-by: Dominic Evans <dominic.evans@uk.ibm.com>

Add missing broker Open() call

014bdc8

Add ReplicationFactor to ListTopics response

0171e3d

birdayz force-pushed the admin_client branch from ed8fc87 to 2ddd27e Compare February 7, 2019 23:32

admin: Add some missing admin methods

3645a34

DescribeCluster DescribeTopic DescribeConsumerGroups ListConsumerGroups ListConsumerGroupOffsets

birdayz force-pushed the admin_client branch from 161c73d to 3645a34 Compare February 8, 2019 00:08

bai merged commit 8733e77 into IBM:master Feb 8, 2019

This was referenced Feb 8, 2019

Add ListTopics func to ClusterAdmin interface #1155

Closed

admin: include ReplicaAssignment in ListTopics() #1274

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

admin: Add some missing admin methods #1178

admin: Add some missing admin methods #1178

birdayz commented Sep 27, 2018 •

edited

Loading

birdayz commented Sep 28, 2018

birdayz commented Oct 14, 2018

jeffwidman commented Jan 4, 2019 •

edited

Loading

birdayz commented Jan 20, 2019

varun06 Jan 22, 2019

birdayz Feb 7, 2019

varun06 Jan 22, 2019

birdayz Feb 7, 2019

varun06 Jan 22, 2019

birdayz Jan 23, 2019

birdayz Feb 7, 2019

birdayz commented Feb 7, 2019 •

edited

Loading

birdayz commented Feb 8, 2019

varun06 commented Feb 8, 2019 via email

birdayz commented Feb 8, 2019

bai commented Feb 8, 2019

bai commented Feb 8, 2019

	_ = b.Open(conf) // Ensure that broker is opened
	b.Open(conf) // Ensure that broker is opened

admin: Add some missing admin methods #1178

admin: Add some missing admin methods #1178

Conversation

birdayz commented Sep 27, 2018 • edited Loading

birdayz commented Sep 28, 2018

birdayz commented Oct 14, 2018

jeffwidman commented Jan 4, 2019 • edited Loading

birdayz commented Jan 20, 2019

varun06 Jan 22, 2019

Choose a reason for hiding this comment

birdayz Feb 7, 2019

Choose a reason for hiding this comment

varun06 Jan 22, 2019

Choose a reason for hiding this comment

birdayz Feb 7, 2019

Choose a reason for hiding this comment

varun06 Jan 22, 2019

Choose a reason for hiding this comment

birdayz Jan 23, 2019

Choose a reason for hiding this comment

birdayz Feb 7, 2019

Choose a reason for hiding this comment

birdayz commented Feb 7, 2019 • edited Loading

birdayz commented Feb 8, 2019

varun06 commented Feb 8, 2019 via email

birdayz commented Feb 8, 2019

bai commented Feb 8, 2019

bai commented Feb 8, 2019

birdayz commented Sep 27, 2018 •

edited

Loading

jeffwidman commented Jan 4, 2019 •

edited

Loading

birdayz commented Feb 7, 2019 •

edited

Loading