-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
MGDSTRM-10039 elevating the priority of broker and zookeeper pods #829
Conversation
- the direct install method however no longer works due to new references to classes not in this bundle
I'm not sure what a good naming convention is here. We already have kas-fleetshard-high. I went with core, but you could also go with standard, moderate, medium high, etc. The choice of 10 for the priority value is also arbitrary. We can of course space that out more if desired - high is 1000000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. I'm not sure about the naming either. I don't object to using core
. Maybe something like operand-high
or similar would be more descriptive?
Since "high" name already set precedent, I like the @shawkins idea of going with "standard, moderate, medium high, etc." then there is no ambiguity when referred in conjunction with other class types. |
|
||
Kafka kafka = kafkaCluster.kafkaFrom(managedKafka, null); | ||
|
||
assertEquals("kas-fleetshard-core", kafka.getSpec().getKafka().getTemplate().getPod().getPriorityClassName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess these need to be updated to new name to fix the unit tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sorry things were incomplete. Should be good now.
also slightly reducing the broker request to make evictions less common
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Kudos, SonarCloud Quality Gate passed! |
also adding some trivial instance profiler changes.