Make settings in cassandra-yaml.yaml and jvm-options.yaml configurable. #1

viivek46 · 2019-10-11T00:31:46Z

What changes are proposed in this PR?

This PR makes setting in both "cassandra-yaml.yml" & "jvm-options.yaml" template configurable, all setting which are configurable in DCOS cassandra for this 2 files are made configurable in this PR.

How were these changes tested?

Manually tested by installing service and checked 'nodetool status'.

operator/templates/jvm-options.yaml

mpereira

Thanks @viivek46. It's looking good. I did a first pass at reviewing and left some comments.

operator/templates/cassandra-yaml.yaml

operator/templates/jvm-options.yaml

operator/params.yaml

mpereira · 2019-10-11T12:04:01Z

I also noticed that some settings in both cassandra-yaml.yaml and jvm-options.yaml remain unconfigurable.

mpereira · 2019-10-11T12:17:24Z

Also, please take a look at the style guide for opening PRs. 🙂

viivek46 · 2019-10-11T15:09:34Z

I also noticed that some settings in both cassandra-yaml.yaml and jvm-options.yaml remain unconfigurable.

Yes i refer our existing framework for that, all which were exposed in our existing framework for this templates i have exposed here, but if you think we should expose more, please let me know.

viivek46 · 2019-10-11T16:05:52Z

Also, please take a look at the style guide for opening PRs.

OK @mpereira , but is there any specific reason why we are removing jira tickets number, as generally we follow this format.

samvantran

Good stuff @viivek46! I left a couple comments but thanks for making all these customizable.

Also I vote that we should put the JIRA ticket in title. Makes it easier to find the ticket/motivation for the changes.

operator/params.yaml

operator/templates/cassandra-yaml.yaml

mpereira · 2019-10-14T10:41:59Z

@viivek46: Yes i refer our existing framework for that, all which were exposed in our existing framework for this templates i have exposed here, but if you think we should expose more, please let me know.

Yes, let's make all settings configurable.

mpereira · 2019-10-14T10:46:36Z

@viivek46: OK @mpereira , but is there any specific reason why we are removing jira tickets number, as generally we follow this format.

Yeah, the JIRA tag in commit messages is useful for us, but here's a couple of reasons I have in mind:

this project isn't related to "DC/OS"
the kudobuilder/operators repository where this operator will be moved to soon doesn't have a practice of tagging commits with JIRA ticket numbers

cc @samvantran

Co-Authored-By: Sam Tran <[email protected]>

for all settings that are originally commented out in the official configuration file, added a conditional block around it

viivek46 · 2019-10-16T03:54:41Z

@mpereira

I have added many more setting configurable, but there are some still left and the reason i haven't added those is because :

Few were conflicting and some were between jvm and cassandra yaml files.
Few configs and related still need to decide what to do about it, like : listen_address
rpc_address
also like few i was sure that it wont work with existing setup like authenticator, authorizer. so haven't made it configurable.
Also for few I was not sure how to add those.

samvantran

Minor nits but looks good to me!

operator/params.yaml

samvantran · 2019-10-16T16:29:11Z

operator/templates/jvm-options.yaml

-    -XX:SurvivorRatio=8
-    -XX:MaxTenuringThreshold=1
-    -XX:CMSInitiatingOccupancyFraction=75
+    {{ if .Params.SURVIVORRATIO }}


If these have default values are were not originally commented, do we still need if checks?

@samvantran Reason i did this because i was not sure how blank value will be treated here, so included in conditional block.

mpereira · 2019-10-16T16:37:47Z

@viivek46: Few were conflicting and some were between jvm and cassandra yaml files

That's a good point. In that case, let's give precedence to the cassandra-yaml.yaml template, and not make them configurable in the jvm-options.yaml. On a managed service like this operator, I don't see a reason why one would need to override a setting via JVM options if it's also available in the configuration file. Could you think of any? cc @samvantran

mpereira

Thanks for the progress so far, and for bearing with our feedback @viivek46.

operator/templates/jvm-options.yaml

mpereira · 2019-10-16T16:43:27Z

I was also wondering if for the JVM options (-XX flags) if it would make sense to prepend something like JVM_OPT_ to the setting names. e.g.: JVM_OPT_MAX_TENURING_THRESHOLD.

That way, Cassandra parameters and JVM options would be more easily distinguished.

What do you think, @viivek46, @samvantran?

viivek46 · 2019-10-16T18:12:20Z

I was also wondering if for the JVM options (-XX flags) if it would make sense to prepend something like JVM_OPT_ to the setting names. e.g.: JVM_OPT_MAX_TENURING_THRESHOLD.

That way, Cassandra parameters and JVM options would be more easily distinguished.

What do you think, @viivek46, @samvantran?

Yea thats better, i have updated PARAM like that.

samvantran · 2019-10-16T19:07:52Z

On a managed service like this operator, I don't see a reason why one would need to override a setting via JVM options if it's also available in the configuration file. Could you think of any? cc @samvantran

my guess is that different settings were incrementally added over time and we're catching these discrepancies bc we're looking at it as a whole with fresh eyes. I agree with Murilo, when in doubt, go with cassandra.yaml as the place to set a config.

I was also wondering if for the JVM options (-XX flags) if it would make sense to prepend something like JVM_OPT_ to the setting names. e.g.: JVM_OPT_MAX_TENURING_THRESHOLD.

💯

mpereira

I've opened a PR with a few fixes and improvements against this PR, please take a look @samvantran and @viivek46 : #2

I think other than the things in the PR above, this PR looks good. The last thing I think we should do is make the authenticator and authorizer configurable. Even though the operator doesn't currently provide any automation for creating users and credentials via cqlsh, it is still possible for users to do it themselves by following the Cassandra docs, so let's make those configurable.

mpereira · 2019-10-18T10:07:30Z

Thanks for the changes @viivek46. Let's wait for @samvantran to have another look at #2 and then we can get both PRs merged!

mpereira · 2019-10-18T10:08:01Z

I'll also run some tests that I have in a wip branch to make sure the operator is still working.

* Add units to setting names, improve names, use correct settings. * Make this look scarier. * Misc improvements. * Better name. * Use fully-scoped actual setting names. * Leave initial_token unconfigurable for now.

mpereira

This PR looks good to me now. I'll approve it, but before merging, please try merging in #3 into your local branch and running the tests, so that we make sure that everything (install/uninstall with no settings) is still working.

Thanks again for your work @viivek46, and your careful reviews @samvantran.

mpereira · 2019-10-18T22:07:39Z

try merging in #3 into your local branch and running the tests, so that we make sure that everything (install/uninstall with no settings) is still working.

I just did this and the tests passed. 🎉

viivek46 added 2 commits October 10, 2019 20:19

added code to make settings in cassandra-yaml.yml configurable

228664b

made few more settings in cassandra-yaml.yml configurable

72dba5d

viivek46 changed the title ~~Make all settings in "cassandra-yaml.yml" & "jvm-options.yaml" template configurable~~ [DCOS-59493] [DCOS-59494] Make settings in "cassandra-yaml.yml" & "jvm-options.yaml" template configurable Oct 11, 2019

added code to make settings in jvm-options.yaml configurable

03be793

viivek46 requested review from mpereira and samvantran October 11, 2019 02:25

viivek46 commented Oct 11, 2019

View reviewed changes

operator/templates/jvm-options.yaml Outdated Show resolved Hide resolved

added line at the end of params.yaml

4aae807

mpereira suggested changes Oct 11, 2019

View reviewed changes

mpereira reviewed Oct 11, 2019

View reviewed changes

operator/params.yaml Outdated Show resolved Hide resolved

mpereira changed the title ~~[DCOS-59493] [DCOS-59494] Make settings in "cassandra-yaml.yml" & "jvm-options.yaml" template configurable~~ Make settings in cassandra-yaml.yaml and jvm-options.yaml configurable. Oct 11, 2019

samvantran reviewed Oct 11, 2019

View reviewed changes

viivek46 and others added 4 commits October 14, 2019 21:25

Apply suggestions from code review

1790aaa

Co-Authored-By: Sam Tran <[email protected]>

added conditional block around originally commented settings

a8aaf6f

for all settings that are originally commented out in the official configuration file, added a conditional block around it

added more settings in cassandra-yaml.yml configurable

8e9559e

added more settings jvm-options.yml configurable

10da9fd

viivek46 requested review from mpereira and samvantran October 16, 2019 03:54

samvantran approved these changes Oct 16, 2019

View reviewed changes

mpereira suggested changes Oct 16, 2019

View reviewed changes

operator/templates/jvm-options.yaml Outdated Show resolved Hide resolved

Apply suggestions from code review

9111b05

viivek46 requested a review from mpereira October 16, 2019 18:10

mpereira reviewed Oct 16, 2019

View reviewed changes

made authenticator/authenticator configurable

121a284

samvantran mentioned this pull request Oct 18, 2019

Cassandra settings improvements. #2

Merged

Cassandra settings improvements. (#2)

661be51

* Add units to setting names, improve names, use correct settings. * Make this look scarier. * Misc improvements. * Better name. * Use fully-scoped actual setting names. * Leave initial_token unconfigurable for now.

mpereira approved these changes Oct 18, 2019

View reviewed changes

viivek46 merged commit 05b09e8 into master Oct 20, 2019

viivek46 deleted the configuration-cassandra-yaml branch October 20, 2019 16:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make settings in cassandra-yaml.yaml and jvm-options.yaml configurable. #1

Make settings in cassandra-yaml.yaml and jvm-options.yaml configurable. #1

viivek46 commented Oct 11, 2019 •

edited

Loading

mpereira left a comment

mpereira commented Oct 11, 2019 •

edited

Loading

mpereira commented Oct 11, 2019

viivek46 commented Oct 11, 2019

viivek46 commented Oct 11, 2019

samvantran left a comment •

edited

Loading

mpereira commented Oct 14, 2019 •

edited

Loading

mpereira commented Oct 14, 2019 •

edited

Loading

viivek46 commented Oct 16, 2019

samvantran left a comment

samvantran Oct 16, 2019

viivek46 Oct 16, 2019

mpereira commented Oct 16, 2019

mpereira left a comment

mpereira commented Oct 16, 2019

viivek46 commented Oct 16, 2019

samvantran commented Oct 16, 2019 •

edited

Loading

mpereira left a comment

mpereira commented Oct 18, 2019

mpereira commented Oct 18, 2019

mpereira left a comment

mpereira commented Oct 18, 2019

Make settings in cassandra-yaml.yaml and jvm-options.yaml configurable. #1

Make settings in cassandra-yaml.yaml and jvm-options.yaml configurable. #1

Conversation

viivek46 commented Oct 11, 2019 • edited Loading

What changes are proposed in this PR?

How were these changes tested?

mpereira left a comment

Choose a reason for hiding this comment

mpereira commented Oct 11, 2019 • edited Loading

mpereira commented Oct 11, 2019

viivek46 commented Oct 11, 2019

viivek46 commented Oct 11, 2019

samvantran left a comment • edited Loading

Choose a reason for hiding this comment

mpereira commented Oct 14, 2019 • edited Loading

mpereira commented Oct 14, 2019 • edited Loading

viivek46 commented Oct 16, 2019

samvantran left a comment

Choose a reason for hiding this comment

samvantran Oct 16, 2019

Choose a reason for hiding this comment

viivek46 Oct 16, 2019

Choose a reason for hiding this comment

mpereira commented Oct 16, 2019

mpereira left a comment

Choose a reason for hiding this comment

mpereira commented Oct 16, 2019

viivek46 commented Oct 16, 2019

samvantran commented Oct 16, 2019 • edited Loading

mpereira left a comment

Choose a reason for hiding this comment

mpereira commented Oct 18, 2019

mpereira commented Oct 18, 2019

mpereira left a comment

Choose a reason for hiding this comment

mpereira commented Oct 18, 2019

viivek46 commented Oct 11, 2019 •

edited

Loading

mpereira commented Oct 11, 2019 •

edited

Loading

samvantran left a comment •

edited

Loading

mpereira commented Oct 14, 2019 •

edited

Loading

mpereira commented Oct 14, 2019 •

edited

Loading

samvantran commented Oct 16, 2019 •

edited

Loading