From b144f22be438dd3432a28670510fa1303329a5ed Mon Sep 17 00:00:00 2001
From: Christian Schwarz <christian@neon.tech>
Date: Tue, 14 May 2024 10:01:30 +0000
Subject: [PATCH] [DNM] matrix run test suite with legacy & tiered compaction

---
 .github/workflows/build_and_test.yml |  8 ++++++--
 libs/pageserver_api/src/models.rs    |  2 ++
 pageserver/src/config.rs             | 17 +++++++++++++++--
 pageserver/src/tenant/config.rs      | 10 +++++++---
 pageserver/src/tenant/timeline.rs    |  3 +++
 5 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/.github/workflows/build_and_test.yml b/.github/workflows/build_and_test.yml
index f8c011a0a579..f57bc4b763e5 100644
--- a/.github/workflows/build_and_test.yml
+++ b/.github/workflows/build_and_test.yml
@@ -432,8 +432,9 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        build_type: [ debug, release ]
-        pg_version: [ v14, v15, v16 ]
+        build_type: [ debug ]
+        pg_version: [ v15 ]
+        pageserver_compaction_algorithm_kind: [ "legacy", "tiered" ]
     steps:
       - name: Checkout
         uses: actions/checkout@v4
@@ -461,6 +462,9 @@ jobs:
           PAGESERVER_GET_VECTORED_IMPL: vectored
           PAGESERVER_GET_IMPL: vectored
           PAGESERVER_VALIDATE_VEC_GET: true
+          PAGESERVER_DEFAULT_TENANT_CONFIG_COMPACTION_ALGORITHM: 'kind="${{ matrix.pageserver_compaction_algorithm_kind }}"'
+          # catch the tests that override `tenant_config` as a whole without specifying the compaction algorithm `kind`
+          NEON_PAGESERVER_PANIC_ON_UNSPECIFIED_COMPACTION_ALGORITHM: true
 
       # Temporary disable this step until we figure out why it's so flaky
       # Ref https://github.com/neondatabase/neon/issues/4540
diff --git a/libs/pageserver_api/src/models.rs b/libs/pageserver_api/src/models.rs
index 9311dab33cd0..fb418380edb4 100644
--- a/libs/pageserver_api/src/models.rs
+++ b/libs/pageserver_api/src/models.rs
@@ -451,6 +451,8 @@ impl EvictionPolicy {
 )]
 #[strum(serialize_all = "kebab-case")]
 pub enum CompactionAlgorithm {
+    #[strum(disabled)]
+    NotSpecified,
     Legacy,
     Tiered,
 }
diff --git a/pageserver/src/config.rs b/pageserver/src/config.rs
index b0afb6414be2..177ee5cd4109 100644
--- a/pageserver/src/config.rs
+++ b/pageserver/src/config.rs
@@ -5,7 +5,7 @@
 //! See also `settings.md` for better description on every parameter.
 
 use anyhow::{anyhow, bail, ensure, Context, Result};
-use pageserver_api::shard::TenantShardId;
+use pageserver_api::{models::CompactionAlgorithm, shard::TenantShardId};
 use remote_storage::{RemotePath, RemoteStorageConfig};
 use serde;
 use serde::de::IntoDeserializer;
@@ -15,7 +15,7 @@ use utils::crashsafe::path_with_suffix_extension;
 use utils::id::ConnectionId;
 use utils::logging::SecretString;
 
-use once_cell::sync::OnceCell;
+use once_cell::sync::{Lazy, OnceCell};
 use reqwest::Url;
 use std::num::NonZeroUsize;
 use std::str::FromStr;
@@ -1067,6 +1067,19 @@ impl PageServerConf {
 
         conf.default_tenant_conf = t_conf.merge(TenantConf::default());
 
+        {
+            const VAR_NAME: &str = "NEON_PAGESERVER_PANIC_ON_UNSPECIFIED_COMPACTION_ALGORITHM";
+            static VAR: Lazy<Option<bool>> = Lazy::new(|| utils::env::var(VAR_NAME));
+            if VAR.unwrap_or(false)
+                && conf.default_tenant_conf.compaction_algorithm.kind
+                    == CompactionAlgorithm::NotSpecified
+            {
+                panic!(
+                        "Unspecified compaction algorithm in default tenant configuration. \
+                        Set the algorithm explicitly in the pageserver.toml's `tenant_config` field or unset the environment variable {VAR_NAME}");
+            }
+        }
+
         Ok(conf)
     }
 
diff --git a/pageserver/src/tenant/config.rs b/pageserver/src/tenant/config.rs
index 342d70595468..33a445099db3 100644
--- a/pageserver/src/tenant/config.rs
+++ b/pageserver/src/tenant/config.rs
@@ -40,8 +40,6 @@ pub mod defaults {
 
     pub const DEFAULT_COMPACTION_PERIOD: &str = "20 s";
     pub const DEFAULT_COMPACTION_THRESHOLD: usize = 10;
-    pub const DEFAULT_COMPACTION_ALGORITHM: super::CompactionAlgorithm =
-        super::CompactionAlgorithm::Legacy;
 
     pub const DEFAULT_GC_HORIZON: u64 = 64 * 1024 * 1024;
 
@@ -554,7 +552,13 @@ impl Default for TenantConf {
                 .expect("cannot parse default compaction period"),
             compaction_threshold: DEFAULT_COMPACTION_THRESHOLD,
             compaction_algorithm: CompactionAlgorithmSettings {
-                kind: DEFAULT_COMPACTION_ALGORITHM,
+                kind: if cfg!(test) {
+                    // Rust tests rely on a valid implicit default (TODO: fix this)
+                    CompactionAlgorithm::Legacy
+                } else {
+                    // Python tests are subject to NotSpecified handling
+                    CompactionAlgorithm::NotSpecified
+                },
             },
             gc_horizon: DEFAULT_GC_HORIZON,
             gc_period: humantime::parse_duration(DEFAULT_GC_PERIOD)
diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs
index d4f6e25843b7..848924cdfade 100644
--- a/pageserver/src/tenant/timeline.rs
+++ b/pageserver/src/tenant/timeline.rs
@@ -1700,6 +1700,9 @@ impl Timeline {
         }
 
         match self.get_compaction_algorithm_settings().kind {
+            CompactionAlgorithm::NotSpecified => {
+                unreachable!("should panic earlier when we construct the default tenant conf")
+            }
             CompactionAlgorithm::Tiered => self.compact_tiered(cancel, ctx).await,
             CompactionAlgorithm::Legacy => self.compact_legacy(cancel, flags, ctx).await,
         }