feature flags: enable EnableAlterDatabaseCreateHiveFirst (#14078)

This is a long awaited improvement in the dedicated database creation process.

`EnableAlterDatabaseCreateHiveFirst` (`enable_alter_database_create_hive_first` in the config) feature flag affects how system tablets of the dedicated database are created (and than managed throughout their lifetime). System tablets (schemeshard, hive, coordinators/mediators, sysview-processor etc) are those that constitute and service the database itself.

Without `EnableAlterDatabaseCreateHiveFirst`, creation of the new dedicated database leaves all it's system tablets in the custody of the main ("root") Hive of the cluster (all other tablets created afterwards live in the custody of the tenant Hive). Database with that separation of responsibilities works fine. The downside is that complete lack of common vision and coordination between two Hives (root and the tenant's) can lead to suboptimal results when database' tablets (both system and others) are balanced under the load. 

With `EnableAlterDatabaseCreateHiveFirst`, creation of the new dedicated database leaves all tenant tablets (except tenant Hive) in the custody of the tenant hive. Which alleviates problem with tablet balancing.

Author: ijon

ydb/core/protos/feature_flags.proto

@@ -104,7 +104,7 @@ message TFeatureFlags {
     optional bool EnableDataShardGenericReadSets = 81 [default = false];
     // enable alter database operation to create subdomain's system tablets
     // directly in subdomain's hive
-    optional bool EnableAlterDatabaseCreateHiveFirst = 82 [default = false];
+    optional bool EnableAlterDatabaseCreateHiveFirst = 82 [default = true];
     reserved 83; // EnableKqpDataQuerySourceRead
     optional bool EnableSmallDiskOptimization = 84 [default = true];
     optional bool EnableDataShardVolatileTransactions = 85 [default = true];

ydb/core/tx/coordinator/coordinator_ut.cpp

@@ -8,6 +8,21 @@
 #include <library/cpp/testing/unittest/registar.h>
 #include <library/cpp/threading/future/async.h>
 
+// ad-hoc test parametrization support: only for single boolean flag
+// taken from ydb/core/ut/common/kqp_ut_common.h:Y_UNIT_TEST_TWIN
+//TODO: introduce general support for test parametrization?
+#define Y_UNIT_TEST_FLAG(N, OPT)                                                                                   \
+    template<bool OPT> void N(NUnitTest::TTestContext&);                                                           \
+    struct TTestRegistration##N {                                                                                  \
+        TTestRegistration##N() {                                                                                   \
+            TCurrentTest::AddTest(#N "-" #OPT "-false", static_cast<void (*)(NUnitTest::TTestContext&)>(&N<false>), false); \
+            TCurrentTest::AddTest(#N "-" #OPT "-true", static_cast<void (*)(NUnitTest::TTestContext&)>(&N<true>), false);   \
+        }                                                                                                          \
+    };                                                                                                             \
+    static TTestRegistration##N testRegistration##N;                                                               \
+    template<bool OPT>                                                                                             \
+    void N(NUnitTest::TTestContext&)
+
 namespace NKikimr::NFlatTxCoordinator::NTest {
 
     using namespace Tests;
@@ -379,7 +394,7 @@ namespace NKikimr::NFlatTxCoordinator::NTest {
             Ydb::Cms::CreateDatabaseRequest,
             Ydb::Cms::CreateDatabaseResponse>;
 
-        Y_UNIT_TEST(RestoreTenantConfiguration) {
+        Y_UNIT_TEST_FLAG(RestoreTenantConfiguration, AlterDatabaseCreateHiveFirst) {
             struct TCoordinatorHooks : public ICoordinatorHooks {
                 bool AllowPersistConfig_ = true;
                 std::unordered_map<ui64, NKikimrSubDomains::TProcessingParams> PersistConfig_;
@@ -397,7 +412,8 @@ namespace NKikimr::NFlatTxCoordinator::NTest {
                 .SetNodeCount(1)
                 .SetDynamicNodeCount(4)
                 .SetUseRealThreads(false)
-                .AddStoragePoolType("ssd");
+                .AddStoragePoolType("ssd")
+                .SetEnableAlterDatabaseCreateHiveFirst(AlterDatabaseCreateHiveFirst);
 
             Tests::TServer::TPtr server = new TServer(serverSettings);
 
@@ -414,6 +430,8 @@ namespace NKikimr::NFlatTxCoordinator::NTest {
             hooks.AllowPersistConfig_ = false;
             hooks.PersistConfig_.clear();
 
+            const ui32 ExpectedProcessingParamsVersion = 2 + (AlterDatabaseCreateHiveFirst ? 1 : 0);
+
             auto createDatabase = [&]() {
                 Ydb::Cms::CreateDatabaseRequest request;
                 request.set_path("/Root/db1");
@@ -445,8 +463,8 @@ namespace NKikimr::NFlatTxCoordinator::NTest {
             UNIT_ASSERT_C(hooks.PersistConfig_.size() > 0, "Expected coordinators to attempt to persist configs");
             std::vector<ui64> coordinators;
             for (auto& pr : hooks.PersistConfig_) {
-                UNIT_ASSERT_C(pr.second.GetVersion() == 2,
-                    "Expected tenant coordinator to have a version 2 config:\n" << pr.second.DebugString());
+                UNIT_ASSERT_C(pr.second.GetVersion() == ExpectedProcessingParamsVersion,
+                    "Expected tenant coordinator to have a version " << ExpectedProcessingParamsVersion << " config:\n" << pr.second.DebugString());
                 coordinators.push_back(pr.first);
             }
 
@@ -460,8 +478,8 @@ namespace NKikimr::NFlatTxCoordinator::NTest {
             runtime.SimulateSleep(TDuration::MilliSeconds(50));
             UNIT_ASSERT_C(hooks.PersistConfig_.size() == coordinators.size(), "Expected all coordinators to persist restored config");
             for (auto& pr : hooks.PersistConfig_) {
-                UNIT_ASSERT_C(pr.second.GetVersion() == 2,
-                    "Expected tenant coordinator to restore a version 2 config:\n" << pr.second.DebugString());
+                UNIT_ASSERT_C(pr.second.GetVersion() == ExpectedProcessingParamsVersion,
+                    "Expected tenant coordinator to restore a version " << ExpectedProcessingParamsVersion << " config:\n" << pr.second.DebugString());
             }
 
             // runtime.SetLogPriority(NKikimrServices::TABLET_MAIN, NActors::NLog::PRI_DEBUG);

ydb/core/tx/schemeshard/ut_serverless/ut_serverless.cpp

@@ -13,9 +13,9 @@ Y_UNIT_TEST_SUITE(TSchemeShardServerLess) {
     Y_UNIT_TEST(Fake) {
     }
 
-    Y_UNIT_TEST(BaseCase) {
+    Y_UNIT_TEST_FLAG(BaseCase, AlterDatabaseCreateHiveFirst) {
         TTestBasicRuntime runtime;
-        TTestEnv env(runtime);
+        TTestEnv env(runtime, TTestEnvOptions().EnableAlterDatabaseCreateHiveFirst(AlterDatabaseCreateHiveFirst));
         ui64 txId = 100;
 
         TestCreateExtSubDomain(runtime, ++txId,  "/MyRoot",
@@ -106,7 +106,17 @@ Y_UNIT_TEST_SUITE(TSchemeShardServerLess) {
                             NLs::PathsInsideDomain(1),
                             NLs::ShardsInsideDomain(0)});
 
-        ui64 sharedHiveTablets = TTestTxConfig::FakeHiveTablets + NKikimr::TFakeHiveState::TABLETS_PER_CHILD_HIVE;
+        // Check that shards of ServerLess0 db are gone after its deletion.
+        //
+        // SharedDB contains:
+        //  - 3 of its own shards: SchemeShard, Coordinator, Mediator
+        //  - 4 of ServerLess0's shards: SchemeShard, Coordinator, Mediator, 1 x DataShard of the table0
+        //
+
+        //NOTE: AlterDatabaseCreateHiveFirst create system tablets in a tenant hive, otherwise they are created in the root hive
+        ui64 sharedHiveTablets = TTestTxConfig::FakeHiveTablets + (AlterDatabaseCreateHiveFirst ? TFakeHiveState::TABLETS_PER_CHILD_HIVE : 1)
+            + 3  // shards of SharedDB
+        ;
         env.TestWaitTabletDeletion(runtime, xrange(sharedHiveTablets, sharedHiveTablets + 4), sharedHive);
     }
 
@@ -299,7 +309,7 @@ Y_UNIT_TEST_SUITE(TSchemeShardServerLess) {
         UNIT_ASSERT(sharedDbSchemeShard != 0
                     && sharedDbSchemeShard != (ui64)-1
                     && sharedDbSchemeShard != TTestTxConfig::SchemeShard);
-                    
+
         TestDescribeResult(DescribePath(runtime, sharedDbSchemeShard, "/MyRoot/SharedDB"),
                            {NLs::PathExist,
                             NLs::ServerlessComputeResourcesMode(EServerlessComputeResourcesModeUnspecified)});
@@ -308,7 +318,7 @@ Y_UNIT_TEST_SUITE(TSchemeShardServerLess) {
             R"(
                 ResourcesDomainKey {
                     SchemeShard: %lu
-                    PathId: 2 
+                    PathId: 2
                 }
                 Name: "ServerLess0"
             )",
@@ -348,7 +358,7 @@ Y_UNIT_TEST_SUITE(TSchemeShardServerLess) {
         TestDescribeResult(DescribePath(runtime, tenantSchemeShard, "/MyRoot/ServerLess0"),
                            {NLs::PathExist,
                             NLs::ServerlessComputeResourcesMode(EServerlessComputeResourcesModeShared)});
-        
+
         auto checkServerlessComputeResourcesMode = [&](EServerlessComputeResourcesMode serverlessComputeResourcesMode) {
             TString alterData = Sprintf(
                 R"(
@@ -406,7 +416,7 @@ Y_UNIT_TEST_SUITE(TSchemeShardServerLess) {
             R"(
                 ResourcesDomainKey {
                     SchemeShard: %lu
-                    PathId: 2 
+                    PathId: 2
                 }
                 Name: "ServerLess0"
             )",
@@ -487,7 +497,7 @@ Y_UNIT_TEST_SUITE(TSchemeShardServerLess) {
             R"(
                 ResourcesDomainKey {
                     SchemeShard: %lu
-                    PathId: 2 
+                    PathId: 2
                 }
                 Name: "ServerLess0"
             )",

ydb/core/tx/schemeshard/ut_serverless/ya.make

@@ -4,6 +4,8 @@ FORK_SUBTESTS()
 
 SIZE(MEDIUM)
 
+TIMEOUT(120)
+
 PEERDIR(
     library/cpp/getopt
     library/cpp/regex/pcre