From c2beee348abe1be293e4e81d2b4afeb4214e9ca0 Mon Sep 17 00:00:00 2001
From: Kamil Braun <kbraun@scylladb.com>
Date: Mon, 4 Sep 2023 17:30:38 +0200
Subject: [PATCH] feature_service: enable `GROUP0_SCHEMA_VERSIONING` in Raft
 mode

As promised in earlier commits:
Fixes: #7620
Fixes: #13957

Also modify two test cases in `schema_change_test` which depend on
the digest calculation method in their checks. Details are explained in
the comments.
---
 gms/feature_service.cc           |  3 +--
 test/boost/schema_change_test.cc | 34 ++++++++++++++++++++++++++++++--
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/gms/feature_service.cc b/gms/feature_service.cc
index 3b1837db2d..1f2b03650f 100644
--- a/gms/feature_service.cc
+++ b/gms/feature_service.cc
@@ -76,13 +76,12 @@ feature_config feature_config_from_db_config(const db::config& cfg, std::set<sst
         }
     }
 
-    fcfg._disabled_features.insert("GROUP0_SCHEMA_VERSIONING"s);
-
     if (!cfg.check_experimental(db::experimental_features_t::feature::ALTERNATOR_STREAMS)) {
         fcfg._disabled_features.insert("ALTERNATOR_STREAMS"s);
     }
     if (!cfg.consistent_cluster_management()) {
         fcfg._disabled_features.insert("SUPPORTS_RAFT_CLUSTER_MANAGEMENT"s);
+        fcfg._disabled_features.insert("GROUP0_SCHEMA_VERSIONING"s);
     }
     if (!cfg.check_experimental(db::experimental_features_t::feature::KEYSPACE_STORAGE_OPTIONS)) {
         fcfg._disabled_features.insert("KEYSPACE_STORAGE_OPTIONS"s);
diff --git a/test/boost/schema_change_test.cc b/test/boost/schema_change_test.cc
index 304b463667..9fd8970c30 100644
--- a/test/boost/schema_change_test.cc
+++ b/test/boost/schema_change_test.cc
@@ -31,6 +31,12 @@
 #include "cdc/cdc_extension.hh"
 #include "utils/UUID_gen.hh"
 
+static cql_test_config disable_raft_schema_config() {
+    cql_test_config c;
+    c.db_config->consistent_cluster_management(false);
+    return c;
+}
+
 SEASTAR_TEST_CASE(test_new_schema_with_no_structural_change_is_propagated) {
     return do_with_cql_env([](cql_test_env& e) {
         return seastar::async([&] {
@@ -139,9 +145,23 @@ SEASTAR_TEST_CASE(test_tombstones_are_ignored_in_version_calculation) {
             auto new_node_version = e.db().local().get_version();
 
             BOOST_REQUIRE_EQUAL(new_table_version, old_table_version);
+
+            // With group 0 schema changes and GROUP0_SCHEMA_VERSIONING, this check wouldn't pass,
+            // because the version after the first schema change is not a digest, but taken
+            // to be the version sent in the schema change mutations; in this case,
+            // `prepare_new_column_family_announcement` took `table_schema->version()`
+            //
+            // On the other hand, the second schema change mutations do not contain
+            // the ususal `system_schema.scylla_tables` mutation (which would contain the version);
+            // they are 'incomplete' schema mutations created by the above piece of code,
+            // not by the usual `prepare_..._announcement` functions. This causes
+            // a digest to be calculated when applying the schema change, and the digest
+            // will be different than the first version sent.
+            //
+            // Hence we use `disable_raft_schema_config()` in this test.
             BOOST_REQUIRE_EQUAL(new_node_version, old_node_version);
         });
-    });
+    }, disable_raft_schema_config());
 }
 
 SEASTAR_TEST_CASE(test_concurrent_column_addition) {
@@ -191,9 +211,19 @@ SEASTAR_TEST_CASE(test_concurrent_column_addition) {
             BOOST_REQUIRE(new_schema->get_column_definition(to_bytes("v3")) != nullptr);
 
             BOOST_REQUIRE(new_schema->version() != old_version);
+
+            // With group 0 schema changes and GROUP0_SCHEMA_VERSIONING, this check wouldn't pass,
+            // because the version resulting after schema change is not a digest, but taken to be
+            // the version sent in the schema change mutations; in this case, `make_update_table_mutations`
+            // takes `s2->version()`.
+            //
+            // This is fine with group 0 where all schema changes are linearized, so this scenario
+            // of merging concurrent schema changes doesn't happen.
+            //
+            // Hence we use `disable_raft_schema_config()` in this test.
             BOOST_REQUIRE(new_schema->version() != s2->version());
         });
-    });
+    }, disable_raft_schema_config());
 }
 
 SEASTAR_TEST_CASE(test_sort_type_in_update) {