Make garbage collection configurable #355

jsanda · 2021-02-11T04:59:10Z

What this PR does:
Makes GC configurable

Which issue(s) this PR fixes:
Fixes #220

Checklist

Changes manually tested
Chart versions updated (if necessary)
Automated Tests added/updated
Documentation added/updated
CLA Signed: DataStax CLA

burmanm · 2021-02-12T08:29:14Z

charts/k8ssandra/values.yaml

@@ -80,6 +80,21 @@ cassandra:
   size:
   newGenSize:

+  gc:


Why this sort of structure? Now we prevent use of ZGC or Shenandoah. I think I'd rather see a ConfigMap or such to make sure the user has to actually understand what they're doing (by having to define the parameters themselves).

We cannot simply use a ConfigMap. We have to work within the constraints of the config-builder since that is what generate the jvm-options for 3.11 or the jvm8-server.options or jvm11-server.options for 4.0. We will need to update config-builder to support additional garbage collectors.

jsanda · 2021-02-15T04:24:23Z

This PR only adds support for CMS and G1. Config files are created by cass-config-builder and that is all it currently supports.

This PR got a lot more involved than I was expecting. See here for an explanation of the logic.

burmanm · 2021-02-15T09:01:34Z

Makefile

@@ -23,6 +23,9 @@ test: fmt vet
 integ-test:
 	go test -v -test.timeout=5m ./tests/integration/... -coverprofile cover.out

+test-gc: fmt vet


Any reason we would need to separate these tests?

I need to add a comment. I didn't want or need to run all the tests and I didn't want to manually keep typing -args -ginkgo.focus=".*garbage\ collection.*". It might be helpful to help additional Makefile targets for running subsets of the tests. I am also fine with removing if you think it is confusing.

Actually, as a total newbie to running these tests for the first time last week I would have loved to at least have an example of how to target the test suite so that I wouldn't have to wait through and sift through the logging for all of them. I think this would be a good add if for nothing else than that purpose.

I would like to add similar, targets for other tests. The verbose output is painful at times. I looked before briefly at cut down the output to just failures but I was unsuccessful.

The alternative (while doing development) is to make the test use a F infront of the test. For example: FContext(.. and then it will only run those focus ones (not others).

The alternative (while doing development) is to make the test use a F infront of the test

Do I need to remove the F if I want to remove the focus? If so, that is not a great general solution.

Yeah. It's intended for development time focusing.

burmanm · 2021-02-15T09:07:09Z

charts/k8ssandra/values.yaml

+  # the datacenter level.
+  gc:
+    # -- GC configuration for the CMS collector.
+    cms: {}


Out of interest, do users have any idea how to configure these in yaml? Or do we need to write documentation to indicate how it's done? I would say it's not clear to the user what the {} part means in this case. Should it remain? Should it be removed if I enable one parameter? Should the parameters be appended inside the {} ?

This is mostly critique to Helm (and yaml of course), but we'll probably need to explain this - users want to install Cassandra in the end, not learn yaml.

These are good questions. I could have used an empty string instead. I chose {} because I have seen that in other charts. To me it indicates that the property has child properties. Will that be clear to the k8ssandra users, particularly those with little or no prior Helm experience? Doubtful. We need to document it.

burmanm · 2021-02-15T09:11:28Z

tests/unit/testdata/cluster-g1-dc-cms-values.yaml

+      waitDuration: 11000
+
+  datacenters:
+    - name: dc1


Just a sidenote, but for future we'll probably want to move this sequence to yaml map. Would make those set commands also so much more simple, like changing just --set datacenter.{dc1}.size=5 instead of having to modify the .yaml file to get that size changed if I want modify a single parameter.

datacenters: "dc1": size: 3 .. etc

Not related to this PR, but I was wondering if we should do that before 1.0 ..

I see the upside of a more dynamic approach. In some regards it is less maintenance. The user is free to add arbitrary properties which don't require any code updates in k8ssandra. It gives the user more flexibility and reduces maintenance burden on the code base in some regards.

There are some downsides though. First, the user has to understand what values are supported by cass-config-builder. It does not expect arbitrary values from cassandra.yaml and from jvm.options.

Secondly, a yaml map would make parsing harder. While users could supply arbitrary values, we would need to verify that those values match what is expected/allow by cass-config-builder.

Ultimately I do think we should provide a hook of some sorts to allow users to set arbitrary values even if it does mean understanding cass-config-builder. I would consider this applicable for advanced use cases.

Lastly, I hope we will be able to put some higher level abstractions in place that could free the user from worrying about some of these settings.

burmanm · 2021-02-18T11:32:56Z

Makefile

@@ -23,6 +23,9 @@ test: fmt vet
 integ-test:
 	go test -v -test.timeout=5m ./tests/integration/... -coverprofile cover.out

+test-gc: fmt vet


The alternative (while doing development) is to make the test use a F infront of the test. For example: FContext(.. and then it will only run those focus ones (not others).

jsanda · 2021-02-21T18:54:12Z

@burmanm @adejanovski can you do another review of this?

Note that the PR exposes what is available through cass-config-definitions which is a subset of all available options.

* initial support for configuring GC * refactor GC related code into helper templates * add support for DC-level config and refactor templates * add tests, big refactoring of k8ssandra.configureGc template * update tests * add more tests, add comments, add helper test target in Makefile * add comments and bump chart version * removing Makefile helper target, see k8ssandra#398

jsanda added this to the 1.0.0 milestone Feb 11, 2021

jsanda added this to In progress in K8ssandra via automation Feb 11, 2021

jsanda self-assigned this Feb 11, 2021

burmanm reviewed Feb 12, 2021

View reviewed changes

jsanda force-pushed the gc-config branch 2 times, most recently from c2c151d to e0da6d7 Compare February 15, 2021 04:15

jsanda requested a review from adejanovski February 15, 2021 04:16

jsanda marked this pull request as ready for review February 15, 2021 04:24

burmanm reviewed Feb 15, 2021

View reviewed changes

burmanm approved these changes Feb 18, 2021

View reviewed changes

K8ssandra automation moved this from In progress to Reviewer approved Feb 18, 2021

jsanda added 7 commits February 19, 2021 18:10

initial support for configuring GC

626b834

refactor GC related code into helper templates

fac1cf2

add support for DC-level config and refactor templates

264eeb3

add tests, big refactoring of k8ssandra.configureGc template

170543d

update tests

76d0d86

add more tests, add comments, add helper test target in Makefile

1066372

add comments and bump chart version

1acad35

jsanda force-pushed the gc-config branch from e0da6d7 to 1acad35 Compare February 21, 2021 18:51

removing Makefile helper target, see k8ssandra#398

18034a0

burmanm merged commit a312289 into k8ssandra:main Feb 22, 2021

K8ssandra automation moved this from Reviewer approved to Done Feb 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make garbage collection configurable #355

Make garbage collection configurable #355

jsanda commented Feb 11, 2021 •

edited

burmanm Feb 12, 2021

jsanda Feb 12, 2021

jsanda commented Feb 15, 2021

burmanm Feb 15, 2021

jsanda Feb 15, 2021

jdonenine Feb 16, 2021

jsanda Feb 17, 2021

burmanm Feb 18, 2021

jsanda Feb 18, 2021

burmanm Feb 19, 2021

burmanm Feb 15, 2021

burmanm Feb 15, 2021

jsanda Feb 18, 2021

burmanm Feb 15, 2021

burmanm Feb 15, 2021

jsanda Feb 18, 2021

burmanm Feb 18, 2021

jsanda commented Feb 21, 2021

Make garbage collection configurable #355

Make garbage collection configurable #355

Conversation

jsanda commented Feb 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsanda commented Feb 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsanda commented Feb 21, 2021

jsanda commented Feb 11, 2021 •

edited