Externalizing of the configuration for kafka cluster creation with pr… #405
Conversation
…evious default values as starting point
| @@ -81,31 +80,17 @@ | |||
| // storage related constants | |||
| private static final double HARD_PERCENT = 0.95; | |||
There was a problem hiding this comment.
There's still an open issue on the hard/soft limits https://issues.redhat.com/browse/MGDSTRM-2836 I don't know however if there was agreement that the formula would just be based upon percentages. At some point we'll want to expand the settings to include that.
| import java.util.TreeMap; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| @ConfigProperties(prefix = "kafka") |
There was a problem hiding this comment.
It might be good if the root property prefix were more unique than kafka - we are kind of already using that for kafka=dev.
Looking a bit ahead it doesn't appear like using multiple configuration profiles (at least with a simplistic map / config property representation) is supported by quarkus - quarkusio/quarkus#8017
There was a problem hiding this comment.
how about "managedkafka"?
There was a problem hiding this comment.
used the "managedkafka" at the root level and collated "kafka" level properties into separate inner class now
| .withXms("3G") | ||
| .withXmx("3G") | ||
| .withXx(JVM_OPTIONS_XX_MAP) | ||
| .withXms(this.config.getJvmXms()) |
There was a problem hiding this comment.
In practice won't the min/max always be the same?
There was a problem hiding this comment.
true, but for flexibility I left as two, I can convert that to single value
There was a problem hiding this comment.
remove the double and only has xms property now
| @@ -451,10 +436,31 @@ private ResourceRequirements getKafkaExporterResources(ManagedKafka managedKafka | |||
| // this could be removed, when we contribute to Sarama to have the support for Elect Leader API | |||
| config.put("leader.imbalance.per.broker.percentage", 0); | |||
|
|
|||
| if(managedKafka.getSpec().getVersions().isStrimziVersionIn(Versions.VERSION_0_22)) { | |||
There was a problem hiding this comment.
This was probably hashed out on the other pr, but shouldn't this be more like a range check rather than in (or not in) a list of versions?
There was a problem hiding this comment.
yes, let us handle this in a separate issue. the version was not strictly semver, so I used something fast to release 0.7.0
| @JsonProperty("kafka.enable-quota") | ||
| protected boolean enableQuota = true; | ||
|
|
||
| @JsonUnwrapped(prefix = "kafka.zoo-keeper.") |
There was a problem hiding this comment.
the "-" is actually weird :-) ... isn't "kafka.zookeeper" better?
There was a problem hiding this comment.
right, that was due to your previous comment to use zookeeper vs zooKeeper the quarkus when camel case uses - in properties
There was a problem hiding this comment.
removed the - and now it is simply zookeeper
| @JsonProperty("kafka.jvm-xx") | ||
| protected String jvmXx = JVM_OPTIONS_XX; | ||
| @JsonProperty("kafka.enable-quota") | ||
| protected boolean enableQuota = true; |
There was a problem hiding this comment.
imho we should have an inner Kafka class as we have for ZooKeeper. As I mention we see as KafkaInstance the overall deployment having Kafka, ZooKeeper, Canary, AdminServer and now even drain cleaner. I would see configuration parameter prefixed with kafka. or zookeeper. or maybe at some point canary. and adminserver. so encapsulating Kafka cluster related config into a specific inner class. Anyway, not a must do, just my view and my 2 cents.
There was a problem hiding this comment.
I started that way, but moved to this model where I thought I was fixing some configuration property confusion, I with later changes that is cleaner
There was a problem hiding this comment.
@ppatierno I reverted to my original model which exactly what you suggest above with Kafka and "ZooKeeperandExporter` classes and have room to grow easily with any additional compontents. The root is now called "managedkafka" and properties now look like
managedkafka.kafka.volume-size=1000Gi
managedkafka.kafka.jvm-xx=ExitOnOutOfMemoryError true
managedkafka.kafka.replicas=3
managedkafka.zookeeper.replicas=3
managedkafka.zookeeper.volume-size=10Gi```
|
@shawkins @ppatierno thanks for your comments give it another rinse. |
There was a problem hiding this comment.
LGTM
@k-wall the implication here is that every value is settable as an individual config property - which in the near-term could be useful.
Longer-term we are assuming broker per node apprach there will be multiple configurations - which would match the environment / instances the operator is deployed into.
For example if you install the operator into a cluster running on 2xlarge, it will have configuration set for 2xlarge installed somehow [1]. This way it will be possible for the operator to manage osd clusters that have different instance types.
[1] currently we just expect the normal quarkus mechanisms to supply the config properties. For the perf tests we will temporarily abuse the logging config override - since that can be used as a full configuration override as well. Ideally though this could have a dedicated config map or even mka settings. Alternatively we may eventually want to try to define multiple configuration profiles in the operator, rather than assuming that something will be providing those values. Assuming homogeneous deployments, the mka spec would then specify the default profile.
…evious default values as starting point