-
Notifications
You must be signed in to change notification settings - Fork 3
Conversation
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.
Very nice @phrynchanka ! Only one minor nit but can we also add a test? Want to make sure this feature continues to works as we make future updates.
@samvantran @mpereira |
Configurable CASSANDRA_YAML_OVERRIDES
dffdd03
to
58ccf74
Compare
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.
Thanks @phrynchanka, this looks great!
I have left some requests, but none of them are blocking.
Do you intend on adding a test for JVM options as well?
Co-Authored-By: Murilo Pereira <[email protected]>
Co-Authored-By: Murilo Pereira <[email protected]>
Co-Authored-By: Murilo Pereira <[email protected]>
Thanks @mpereira !
Yep. Now working on it. |
Co-Authored-By: Murilo Pereira <[email protected]>
Test for jvm options done. |
"github.com/mesosphere/kudo-cassandra-operator/tests/utils/cassandra" | ||
"github.com/mesosphere/kudo-cassandra-operator/tests/utils/k8s" | ||
"github.com/mesosphere/kudo-cassandra-operator/tests/utils/kubectl" | ||
"github.com/mesosphere/kudo-cassandra-operator/tests/utils/kudo" |
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'm curious about this change.
return getConfigurationFromNodeLogs(namespaceName, instanceName, "o.a.c.service.CassandraDaemon - JVM Arguments: \\[(.+)\\]", ",") | ||
} | ||
|
||
func getConfigurationFromNodeLogs( |
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.
Nice approach.
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.
Thanks for the great work @phrynchanka.
I've opened a PR on top of this PR with a few formatting and naming changes. Nothing blocking, but please take a look: #20
* Move cassandra.yaml related parameter to its block. * Improve test name. * Fix custom JVM options test name. * Improve function name. * Enforce 80 lines and consistency with line breaks. * The KUDO dependency updated, reflect that.
What changes are proposed in this PR?
Resolves DCOS-59496
Resolves DCOS-59495
This PR Implement adding:
Implementation steps :
CUSTOM_CASSANDRA_YAML_BASE64
andCUSTOM_JVM_OPTIONS_BASE64
properties toparams.yml
jvm-options.yaml
andcassandra-yaml.yaml
.How were these changes tested?
Manually tested
cassandra-node-0
from k8s ui. Onexec
consolecat
the content ofetc/cassadra/cassanda.yaml
andetc/cassadra/jvm.options
and see that properties were decoded and propagated.