Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable new config builder for 4.1 deployments #540

Merged
merged 17 commits into from
Aug 16, 2023

Conversation

burmanm
Copy link
Contributor

@burmanm burmanm commented Jun 20, 2023

What this PR does:
Replaces the usage of cass-config-builder with the new k8ssandra-client config build when a Cassandra 4.1 and newer OSS image is deployed.

The old behavior remains with DSE and older versions of Cassandra.

Which issue(s) this PR fixes:
Fixes #512
Fixes #513
Fixes #514

Checklist

  • Changes manually tested
  • Automated Tests added/updated
  • Documentation added/updated
  • CHANGELOG.md updated (not required for documentation PRs)
  • CLA Signed: DataStax CLA

@burmanm burmanm marked this pull request as ready for review July 24, 2023 13:17
@burmanm burmanm requested a review from a team as a code owner July 24, 2023 13:17
@burmanm
Copy link
Contributor Author

burmanm commented Jul 25, 2023

I just realized this isn't going to test the new stuff in the CI since it overrides the version in the GHA workflow. Dang.

@burmanm burmanm changed the title Test runs for new config-builder Enable new config builder for 4.1 deployments Jul 25, 2023
@adejanovski adejanovski self-requested a review July 26, 2023 08:10
Copy link
Contributor

@adejanovski adejanovski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like there are some errors in the new checks.
I just have one blocking request around the image coordinates, and the rest looks good.
Very cool to finally be replacing cass-config-builder!

@@ -953,3 +954,7 @@ func (dc *CassandraDatacenter) DatacenterName() string {
}
return dc.Name
}

func (dc *CassandraDatacenter) UseClientImage() bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Some comments or a different method name could help understanding the intent here. Currently I got confused by what this is supposed to support as feature (no mention of config building for example).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I just lacked imagination when creating the method. Perhaps something like "NewConfigSystem() bool" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean the real idea is to check if the deployed Cassandra version uses old Config.java or the newer one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NewConfigSystem() sounds better. Maybe Builder would be better than System?

The idea indeed is that with versions equal or higher than 4.1, we'll use the new config builder that sits in the k8ssandra-client project.
New is a little vague as well, because if we come up with another one later (god forbids) we'll end up with NewNew ? :D
Is UseK8ssandraConfigBuilder() too long? You could also turn it into K8ssandraConfigBuilder() bool, which may work with Go idioms I guess?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I didn't mean to reflect on the newer config-builder, I was thinking about from the perspective of Cassandra itself. That said, of course Cassandra's Config system can evolve as well later.

config/manager/image_config.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@adejanovski adejanovski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, pending passing tests

@burmanm
Copy link
Contributor Author

burmanm commented Aug 15, 2023

There's something super fishy with the size = 2 and role creation in 4.1 settings.

INFO  [epollEventLoopGroup-5-3] 2023-08-15 08:04:37,693 RpcExecutionException.java:34 - Failed to execute method NodeOps.createRole
org.apache.cassandra.exceptions.UnavailableException: Cannot achieve consistency level EACH_QUORUM in DC My_Super_Dc

@emerkle826
Copy link
Contributor

There's something super fishy with the size = 2 and role creation in 4.1 settings.

INFO  [epollEventLoopGroup-5-3] 2023-08-15 08:04:37,693 RpcExecutionException.java:34 - Failed to execute method NodeOps.createRole
org.apache.cassandra.exceptions.UnavailableException: Cannot achieve consistency level EACH_QUORUM in DC My_Super_Dc

Is this an issue where creating Roles use a special CL that is different than the effective CL for other queries? Or is it just a weird thing specifically with exactly 2 nodes (i,.e things work with 1 node, or 3+ nodes, but not 2)?

@burmanm
Copy link
Contributor Author

burmanm commented Aug 15, 2023

Is this an issue where creating Roles use a special CL that is different than the effective CL for other queries? Or is it just a weird thing specifically with exactly 2 nodes (i,.e things work with 1 node, or 3+ nodes, but not 2)?

The number 2.. but hopefully that's solved now.

@burmanm burmanm merged commit 6b94f47 into k8ssandra:master Aug 16, 2023
36 of 38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enrich configs Add defaults if values are missing Replace cass-config-builder with Go
3 participants