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 topic configuration settings to KafkaTopic CR status #5758

Closed
katheris opened this issue Oct 21, 2021 · 9 comments
Closed

Add topic configuration settings to KafkaTopic CR status #5758

katheris opened this issue Oct 21, 2021 · 9 comments

Comments

@katheris
Copy link
Contributor

Is your feature request related to a problem? Please describe.
As a user when I create a KafkaTopic it would be useful to see what configuration has been applied to the topic as some of these values may be set based on either Kafka or Strimzi defaults. This issue is being created based on a discussion in the community call on 21st October 2021.

Describe the solution you'd like
Add configuration details such as replication factor and minimum insync replicas into the KafkaTopic CR status field.

Describe alternatives you've considered
As an alternative developers could specify these values themselves in the CR so that they don't use any defaults

Additional context
Add any other context or screenshots about the feature request here.

@katheris
Copy link
Contributor Author

This feature suggestion was originally discussed in the community call as a result of discussions around issue #5681. However upon further investigation I noted that the KafkaTopic CRD contains a minimum value for partitions and replication factor. https://github.com/strimzi/strimzi-kafka-operator/blob/main/packaging/install/cluster-operator/043-Crd-kafkatopic.yaml#L62 I believe this means that the default is actually never taken if developers use the KafkaTopic CR. I tested this theory by creating a KafkaTopic with replicas and partitions omitted and found that my resulting CR did have them both set to 1. This reduces the usefulness of this enhancement as it means the defaults for partitions and replication factor are ignored. However this enhancement may still be useful for other configuration options such as min-insync replicas.

@scholzj
Copy link
Member

scholzj commented Oct 21, 2021

@katheris The replicas and partitions fields should be now optional after #4711

@katheris
Copy link
Contributor Author

@katheris The replicas and partitions fields should be now optional after #4711

I believe my tests were against that version of the CRD. I do not see the required section in the KafkaTopics CRD on my system, but when I create a topic without replicas and partitions Kubernetes is adding it in with a value of 1.

@scholzj
Copy link
Member

scholzj commented Oct 21, 2021

I believe my tests were against that version of the CRD. I do not see the required section in the KafkaTopics CRD on my system, but when I create a topic without replicas and partitions Kubernetes is adding it in with a value of 1.

Hmm, I guess it was not properly tested in that case. The idea was that you don't specify these fields and without them the Kafka cluster defaults will be used. :-(

@tombentley
Copy link
Member

@katheris had you configured the Kafka cluster's default's for topic replicas and partitions when you tested this?

The TO is a bit weird in that it doesn't follow the usual operator pattern of treating the spec as read-only and using the status to reflect the current state. It tries to update the spec because it's bidirectional, so allowing you to create and modify topics directly in Kafka, and having the KafkaTopic show the correct state. It's worked like that since before CRDs supported custom status. I don't think it can work any other way if it's going to support updates happening on the Kafka side, otherwise those updates would get reverted by the operator, because the spec would never change.

@tombentley
Copy link
Member

tl;dr On reflection, I think adding this info to status will likely confuse people since the spec should already show it.

@katheris
Copy link
Contributor Author

Ah @tombentley I hadn't realised the topic operator would update the spec, I had assumed it was Kubernetes filling it in. I will have another go with the topic operator turned off and see what the behaviour is. But if you are correct and the topic operator will update the spec with the value that is being used, then I agree that perhaps we don't need it in the status.

@scholzj
Copy link
Member

scholzj commented Dec 9, 2021

@katheris Did you had a chance to look into it? Is this still something we want / need?

@katheris
Copy link
Contributor Author

Hey @scholzj, I've tested this and without the topic operator enabled I can create a KafkaTopic with no partitions or replicas, then once the topic operator comes up it puts in the values. So I think Tom is right that we don't need this because the user can just look at the KafkaTopic spec to determine the partitions and replicas

@scholzj scholzj closed this as completed Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants