ClusterAdmin Support #1055

chandradeepak · 2018-02-16T03:27:31Z

@eapache @buyology @Mongey @mimaison @ongardie-ebay

I started working on ClusterAdmin support and wrote an initial draft version. Want to get your comments that I am moving in the right direction. After your comments i will close to the pull request.

eapache · 2018-02-16T14:29:39Z

admin.go

+	}
+
+	// Check that we are not dealing with a closed Client before processing any other arguments
+	if client.Closed() {


unnecessary since you literally just created it

Will delete that

eapache · 2018-02-16T14:29:59Z

admin.go

+	return ca, nil
+}
+
+func (ca *clusterAdmin) handleResponses(rsp interface{}) {


I don't understand the motivation for grouping all the response-handling in a giant switch here

I will handle all the responses . For now I clubbed it but I will handle them individually.

eapache · 2018-02-16T14:30:28Z

client.go

@@ -35,6 +35,8 @@ type Client interface {
 	// topic/partition, as determined by querying the cluster metadata.
 	Leader(topic string, partitionID int32) (*Broker, error)

+	Any() *Broker


I don't think there's a good reason to expose this publicly, it's never been a well-defined method in the first place.

Will use client.Brokers instead of any.

chandradeepak · 2018-02-19T06:28:15Z

@eapache what is the convenient way to access broker (CreateTopic , DeleteTopic from client. I exposed Any() from client interface but i agree it is not needed.I tried using brokers but it is not guaranteed they are open always. My tests failed with broker not connected. Can you please suggest any other way to access the underlying brokers which are opened . I will make those changes .

buyology · 2018-02-19T09:53:03Z

I would suggest that we first settle on the design in #1048 before finalising the implementation.

Also, part of the answer to the question about how to access the broker is that some of the methods (CreateTopics, DeleteTopics, CreatePartitions) must go through the controller — and the method for retrieving the controller is not yet implemented.

chandradeepak · 2018-02-19T09:56:38Z

Thanks @buyology , will wait for the design to be finalized and method for retrieving the controller.

chandradeepak · 2018-03-20T06:44:28Z

@eapache @buyology , I integrated the controller for this pull request. let me know if you have any comments or changes that needs to be done .

eapache · 2018-06-14T19:24:22Z

admin.go

+// ClusterAdmin is the administrative client for Kafka, which supports managing and inspecting topics,
+// brokers, configurations and ACLs. The minimum broker version required is 0.10.0.0.
+// Methods with stricter requirements will specify the minimum broker version required.
+// This client was introduced in 0.11.0.0 and the API is still evolving.


I don't think this line is necessary in the documentation?

ok. will remove that . I was thinking mentioning the version dependency would be needed especially admin related stuff is supported only in latest versions of kafka. i just followed how we have description on brokers package.

eapache · 2018-06-14T19:24:40Z

admin.go

+// brokers, configurations and ACLs. The minimum broker version required is 0.10.0.0.
+// Methods with stricter requirements will specify the minimum broker version required.
+// This client was introduced in 0.11.0.0 and the API is still evolving.
+// You MUST call Close() on a client to avoid leaks


s/client/cluster admin

eapache · 2018-06-14T19:24:51Z

admin.go

+// This client was introduced in 0.11.0.0 and the API is still evolving.
+// You MUST call Close() on a client to avoid leaks
+type ClusterAdmin interface {
+	// Creates a new topic .This operation is supported by brokers with version 0.10.1.0 or higher.


typo on the period

will address that.

eapache · 2018-06-14T19:25:35Z

admin.go

+// This client was introduced in 0.11.0.0 and the API is still evolving.
+// You MUST call Close() on a client to avoid leaks
+type ClusterAdmin interface {
+	// Creates a new topic .This operation is supported by brokers with version 0.10.1.0 or higher.


You don't need to specify the version requirement here, I prefer it at the end of the description where it already is for other methods.

ok , will do that.

eapache · 2018-06-14T19:25:58Z

admin.go

+	// The validateOnly option is supported from version 0.10.2.0.
+	CreateTopic(topic string, detail *TopicDetail) error
+
+	// Delete a batch of topics.


the method only takes one topic

that is what we though in the initial design. So i went with that . Should i change it to take multiple topics ?

Probably? At the very least, the code and the documentation should be consistent.

changed the comments to singular topic.

eapache · 2018-06-14T19:27:17Z

admin.go

+	// to become aware that the topics have been created. During this time, ClusterAdmin#listTopics
+	// and ClusterAdmin#describeTopics may not return information about the new topics.
+	// This operation is supported by brokers with version 0.10.1.0 or higher.
+	// The validateOnly option is supported from version 0.10.2.0.


the method doesn't take a validateOnly flag

gave option for validateOnly flag

eapache · 2018-06-14T19:28:04Z

admin.go

+	// Creates access control lists (ACLs) which are bound to specific resources.
+	// This operation is not transactional so it may succeed for some ACLs while fail for others.
+	// If you attempt to add an ACL that duplicates an existing ACL, no error will be raised, but
+	// no changes will be made.This operation is supported by brokers with version 0.11.0.0 or higher.


missing space

eapache · 2018-06-14T19:28:10Z

admin.go

+	// no changes will be made.This operation is supported by brokers with version 0.11.0.0 or higher.
+	CreateACL(resource Resource, acl Acl) error
+
+	ListAcls(filter AclFilter) ([]ResourceAcls, error)


missing description

gave support for ListAcls and added the description for it.

eapache · 2018-06-14T19:30:42Z

admin.go

+	}
+
+	if detail == nil {
+		return ErrInvalidInput


I don't think it's worth naming an error for this, it's better to give something less computer-friendly and more human-friendly (e.g. errors.New("You must specify topic details"))

ok sure , will do that

removed the ErrInvalidInput from errors.go

eapache · 2018-06-15T12:37:57Z

Did you intend to push #1116 to this PR instead?

eapache · 2018-06-15T12:39:22Z

admin.go

+
+import "errors"
+
+type ClusterAdmin interface {


Sorry my previous comment wasn't clear. I still think we should have a description for ClusterAdmin, I just think it doesn't need to mention the bit about "This client was introduced in 0.11.0.0 and the API is still evolving."

eapache · 2018-06-15T12:40:22Z

admin.go

+import "errors"
+
+type ClusterAdmin interface {
+	// Creates a new topic. This operation is supported by brokers with version 0.10.1.0 or higher.


You've still got a duplicate version sentence here.

eapache · 2018-06-15T12:41:28Z

admin.go

+	// The validateOnly option is supported from version 0.10.2.0.
+	CreateTopic(topic string, detail *TopicDetail, validateOnly bool) error
+
+	// Delete a batch of topics.


This sentence needs to be singular still since the method only deletes one at a time.

eapache · 2018-06-15T12:41:50Z

admin.go

+	// new partitions. This operation is supported by brokers with version 1.0.0 or higher.
+	CreatePartitions(topic string, count int32, assignment [][]int32, validateOnly bool) error
+
+	//Delete records whose offset is smaller than the given offset of the corresponding partition.


spacing after the //

eapache · 2018-06-15T12:43:32Z

errors.go

@@ -49,6 +49,8 @@ var ErrControllerNotAvailable = errors.New("kafka: controller is not avaiable")
 // the metadata.
 var ErrNoTopicsToUpdateMetadata = errors.New("kafka: no specific topics to update metadata")

+var ErrReferenceNodeNotFound = errors.New("kafka: response did not contain a reference to node")


Is this fundamentally any different from ErrIncompleteResponse?

i was looking at the java documentation in kafka when ever topics creation is not successful or the response we dont get a response to the topic which we requested changes for it was returning some thing like that . I can change it to use ErrIncompleteResponse but it doesn't say the intent that the specified resource is not found. I will wait for your response and take that action.

I'm not sure I fully understand, if something doesn't work the broker should still reply with that topic/partition, just with an error code set? If it's omitting the topic/partition entirely that's new behaviour.

At least as written, NodeNotFound doesn't contain any more information beyond IncompleteResponse about the "why", so I'd prefer to stick with IncompleteResponse until we have something more meaningful to put here.

ok sure will switch to do that .

eapache · 2018-06-15T18:40:25Z

This looks pretty good to me now. Could you please squash/rebase your commits?

chandradeepak · 2018-06-15T22:35:01Z

Done squashing all the commits .

eapache · 2018-06-15T22:43:06Z

Thanks!

eapache reviewed Feb 16, 2018

View reviewed changes

chandradeepak changed the title ~~Draft version of ClusterAdmin Support~~ ClusterAdmin Support Feb 19, 2018

eapache force-pushed the master branch from ecf3de7 to eae9146 Compare February 26, 2018 14:34

chandradeepak force-pushed the master branch 2 times, most recently from b12bde3 to ce27e4e Compare March 17, 2018 02:47

chandradeepak mentioned this pull request Apr 6, 2018

Create topic support at sarama.Client level instead of sarama.Broker level. #1039

Closed

eapache reviewed Jun 14, 2018

View reviewed changes

eapache reviewed Jun 15, 2018

View reviewed changes

support of admin operations in sarama

3202295

chandradeepak force-pushed the master branch from ff8a741 to 3202295 Compare June 15, 2018 22:04

eapache merged commit 46cf3e2 into IBM:master Jun 15, 2018

This was referenced Aug 22, 2018

ClusterAdmin swallows TopicError ErrMsg #1153

Closed

Add ListTopics func to ClusterAdmin interface #1155

Closed

ClusterAdmin Support #1055

ClusterAdmin Support #1055

Conversation

chandradeepak commented Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chandradeepak commented Feb 19, 2018

buyology commented Feb 19, 2018 • edited Loading

chandradeepak commented Feb 19, 2018

chandradeepak commented Mar 20, 2018

Choose a reason for hiding this comment

chandradeepak Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eapache commented Jun 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eapache commented Jun 15, 2018

chandradeepak commented Jun 15, 2018

eapache commented Jun 15, 2018

chandradeepak commented Feb 16, 2018 •

edited

Loading

buyology commented Feb 19, 2018 •

edited

Loading

chandradeepak Jun 14, 2018 •

edited

Loading