CASSCPP-15: Add support for Cassandra 4.1.x and 5.0 releases to CI #592
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,6 +62,7 @@ docs | |
| .#* | ||
| #* | ||
| .clang_complete | ||
| .idea | ||
|
|
||
| # OSX | ||
| .DS_Store | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1488,19 +1488,58 @@ CCM::Bridge::generate_create_updateconf_command(CassVersion cassandra_version) { | |
| updateconf_command.push_back("enable_user_defined_functions:true"); | ||
| } | ||
|
|
||
|
Comment on lines
1488
to
-1493
Contributor
Author
There was a problem hiding this comment. This doesn't seem to be in option in Cassandra 5.0+ but as far as I can tell we don't actually used scripted UDFs anyways so we can just delete it
Contributor
There was a problem hiding this comment. It's still defined, it's just been renamed because of the conversion that happened with C* 4.1 (the reason we have to do all this translation. The reference is here. You should be able to keep this in and just let the for loop calling translate_config_for_version() below take care of this.
Contributor
Author
There was a problem hiding this comment. If you look at https://apache.googlesource.com/cassandra/+/trunk/src/java/org/apache/cassandra/config/DatabaseDescriptor.java, you'll see enable_scripted_user_defined_functions was actually removed in 5.0+. But like I said we don't use JS UDFs anyways so we don't need it in the first place (of course if I'm wrong do lmk) |
||
| if (cassandra_version >= "4.0.0" && !is_dse()) { | ||
| updateconf_command.push_back("enable_materialized_views:true"); | ||
| updateconf_command.push_back("enable_user_defined_functions:true"); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Boy, there are a lot of hard-coded cassandra.yaml configs that get updated here for very obsolete C* versions. I see fixed config settings for pre-2.0 C*... yikes. Those can all be deleted, or at least the ones for anything before C* 4.0 can be (assuming we're using OSS C* as our minimum here as discussed in another comment). Specifically I'm thinking all of these can be removed and these can just be made part of the default.
Contributor
Author
There was a problem hiding this comment. lol alright will do
Contributor
Author
There was a problem hiding this comment. Will put this on hold until we decide exactly which versions we want to keep testing against |
||
|
|
||
| for (size_t i = 0; i < updateconf_command.size(); ++i) { | ||
| updateconf_command[i] = translate_config_for_version(updateconf_command[i], cassandra_version); | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Semi-nit: this forces us to loop through the array twice, once when creating it and again to update everything. Maybe just create a helper function which calls
Contributor
Author
There was a problem hiding this comment. I did that at first but because there's no nested functions and I didn't see lambdas used much in many other areas of the codebase (i.e. so no clean closures or currying) I just stuck with this way because I didn't like having to pass all the context into the translation function at every call. It's not performance sensitive nor is it a lot of elements so it's fine in this context. If we wanted pure performance there's plenty more we could do here beyond solving O(2N) 🙂 |
||
| return updateconf_command; | ||
| } | ||
|
|
||
| std::string CCM::Bridge::translate_config_for_version(const std::string& key_value, | ||
| CassVersion cassandra_version) { | ||
| if (cassandra_version < "4.1.0") { | ||
| return key_value; | ||
| } | ||
|
|
||
| std::size_t separator = key_value.find(':'); | ||
| if (separator == std::string::npos) { | ||
| return key_value; | ||
| } | ||
|
|
||
| std::string key = key_value.substr(0, separator); | ||
| std::string value = key_value.substr(separator + 1); | ||
|
|
||
| if (key.find('.') != std::string::npos) { | ||
| return key_value; | ||
| } | ||
|
|
||
| static const char* SUFFIXES[][2] = { | ||
| { "_in_ms", "ms" }, | ||
| { "_in_mb", "MiB" }, | ||
| { "_mb_per_sec", "MiB/s" } | ||
| }; | ||
|
|
||
| for (const auto& i : SUFFIXES) { | ||
| std::string suffix = i[0]; | ||
| if (key.size() >= suffix.size() && | ||
|
Contributor
Author
There was a problem hiding this comment. I noticed there was are Which I still could if you think it's cleaner to be honest. |
||
| key.compare(key.size() - suffix.size(), suffix.size(), suffix) == 0) { | ||
| return key.substr(0, key.size() - suffix.size()) + ":" + value + i[1]; | ||
| } | ||
| } | ||
|
|
||
| std::string enable_prefix = "enable_"; | ||
| if (key.compare(0, enable_prefix.size(), enable_prefix) == 0) { | ||
| return key.substr(enable_prefix.size()) + "_enabled:" + value; | ||
| } | ||
|
|
||
| return key_value; | ||
| } | ||
|
|
||
| std::string CCM::Bridge::generate_dse_workloads(std::vector<DseWorkload> workloads) { | ||
| std::string dse_workloads; | ||
| for (std::vector<DseWorkload>::iterator iterator = workloads.begin(); iterator != workloads.end(); | ||
|
|
||
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.
Not sure which exact versions we want to test so I just left them all here for now...
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.
We normally aim to support platforms (operating systems, JDK versions, Python runtimes etc.) that aren't EOL at time of release. That policy doesn't apply directly to the underlying Cassandra version but in practice we basically do the same. So Java currently supports C* 4.0 and up (since everything older is EOL) and my plan was to do the same here.
DSE certainly complicates that story. I haven't done a look recently to see where we stand on supported DSE versions but I think DSE 5.1, 6.8 and prolly 6.9 is likely adequate. Would be nice if we could get HCD 1.0 in there as well... did you do any testing with that?