Merge pull request #46478 from ClickHouse/fix_test_store_cleanup

Fix some flaky integration tests

programs/local/LocalServer.cpp

@@ -654,8 +654,9 @@ void LocalServer::processConfig()
 
         if (!config().has("only-system-tables"))
         {
+            DatabaseCatalog::instance().createBackgroundTasks();
             loadMetadata(global_context);
-            DatabaseCatalog::instance().loadDatabases();
+            DatabaseCatalog::instance().startupBackgroundCleanup();
         }
 
         /// For ClickHouse local if path is not set the loader will be disabled.

programs/server/Server.cpp

@@ -1656,11 +1656,12 @@ try
         /// that may execute DROP before loadMarkedAsDroppedTables() in background,
         /// and so loadMarkedAsDroppedTables() will find it and try to add, and UUID will overlap.
         database_catalog.loadMarkedAsDroppedTables();
+        database_catalog.createBackgroundTasks();
         /// Then, load remaining databases
         loadMetadata(global_context, default_database);
         convertDatabasesEnginesIfNeed(global_context);
         startupSystemTables();
-        database_catalog.loadDatabases();
+        database_catalog.startupBackgroundCleanup();
         /// After loading validate that default database exists
         database_catalog.assertDatabaseExists(default_database);
         /// Load user-defined SQL functions.

src/Interpreters/DatabaseCatalog.cpp

@@ -152,20 +152,30 @@ void DatabaseCatalog::initializeAndLoadTemporaryDatabase()
     attachDatabase(TEMPORARY_DATABASE, db_for_temporary_and_external_tables);
 }
 
-void DatabaseCatalog::loadDatabases()
+void DatabaseCatalog::createBackgroundTasks()
 {
+    /// It has to be done before databases are loaded (to avoid a race condition on initialization)
     if (Context::getGlobalContextInstance()->getApplicationType() == Context::ApplicationType::SERVER && unused_dir_cleanup_period_sec)
     {
         auto cleanup_task_holder
             = getContext()->getSchedulePool().createTask("DatabaseCatalog", [this]() { this->cleanupStoreDirectoryTask(); });
         cleanup_task = std::make_unique<BackgroundSchedulePoolTaskHolder>(std::move(cleanup_task_holder));
+    }
+
+    auto task_holder = getContext()->getSchedulePool().createTask("DatabaseCatalog", [this](){ this->dropTableDataTask(); });
+    drop_task = std::make_unique<BackgroundSchedulePoolTaskHolder>(std::move(task_holder));
+}
+
+void DatabaseCatalog::startupBackgroundCleanup()
+{
+    /// And it has to be done after all databases are loaded, otherwise cleanup_task may remove something that should not be removed
+    if (cleanup_task)
+    {
         (*cleanup_task)->activate();
         /// Do not start task immediately on server startup, it's not urgent.
         (*cleanup_task)->scheduleAfter(unused_dir_hide_timeout_sec * 1000);
     }
 
-    auto task_holder = getContext()->getSchedulePool().createTask("DatabaseCatalog", [this](){ this->dropTableDataTask(); });
-    drop_task = std::make_unique<BackgroundSchedulePoolTaskHolder>(std::move(task_holder));
     (*drop_task)->activate();
     std::lock_guard lock{tables_marked_dropped_mutex};
     if (!tables_marked_dropped.empty())

src/Interpreters/DatabaseCatalog.h

@@ -135,8 +135,9 @@ public:
     static DatabaseCatalog & instance();
     static void shutdown();
 
+    void createBackgroundTasks();
     void initializeAndLoadTemporaryDatabase();
-    void loadDatabases();
+    void startupBackgroundCleanup();
     void loadMarkedAsDroppedTables();
 
     /// Get an object that protects the table from concurrently executing multiple DDL operations.

tests/integration/test_broken_detached_part_clean_up/test.py

@@ -222,9 +222,11 @@ def test_store_cleanup(started_cluster):
     node1.wait_for_log_line(
         "Removing access rights for unused directory",
         timeout=60,
-        look_behind_lines=1000,
+        look_behind_lines=1000000,
+    )
+    node1.wait_for_log_line(
+        "directories from store", timeout=60, look_behind_lines=1000000
     )
-    node1.wait_for_log_line("directories from store", look_behind_lines=1000)
 
     store = node1.exec_in_container(["ls", f"{path_to_data}/store"])
     assert "100" in store
@@ -277,10 +279,14 @@ def test_store_cleanup(started_cluster):
     )
 
     node1.wait_for_log_line(
-        "Removing unused directory", timeout=90, look_behind_lines=1000
+        "Removing unused directory", timeout=90, look_behind_lines=1000000
+    )
+    node1.wait_for_log_line(
+        "directories from store", timeout=90, look_behind_lines=1000000
+    )
+    node1.wait_for_log_line(
+        "Nothing to clean up from store/", timeout=90, look_behind_lines=1000000
     )
-    node1.wait_for_log_line("directories from store")
-    node1.wait_for_log_line("Nothing to clean up from store/")
 
     store = node1.exec_in_container(["ls", f"{path_to_data}/store"])
     assert "100" in store

tests/integration/test_dictionaries_all_layouts_separate_sources/test_cassandra.py

@@ -2,6 +2,11 @@ import os
 import math
 import pytest
 
+# FIXME This test is too flaky
+# https://github.com/ClickHouse/ClickHouse/issues/33006
+
+pytestmark = pytest.mark.skip
+
 from .common import *
 
 from helpers.cluster import ClickHouseCluster