From dd619913da1349d3587cf864114450c01bd5551f Mon Sep 17 00:00:00 2001 From: bwbonanno Date: Thu, 19 Oct 2023 12:45:57 -0700 Subject: [PATCH] Use RwLock to never persist cli state to db --- index-scheduler/src/error.rs | 4 ++++ index-scheduler/src/features.rs | 40 ++++++++++++++++++++----------- index-scheduler/src/lib.rs | 3 +-- meilisearch/tests/features/mod.rs | 8 +++++++ 4 files changed, 39 insertions(+), 16 deletions(-) diff --git a/index-scheduler/src/error.rs b/index-scheduler/src/error.rs index 417a55be8..d901129d2 100644 --- a/index-scheduler/src/error.rs +++ b/index-scheduler/src/error.rs @@ -127,6 +127,8 @@ pub enum Error { Persist(#[from] tempfile::PersistError), #[error(transparent)] FeatureNotEnabled(#[from] FeatureNotEnabledError), + #[error("An unexpected error occurred when accessing the runtime features.")] + RuntimeFeatureToggleError, #[error(transparent)] Anyhow(#[from] anyhow::Error), @@ -186,6 +188,7 @@ impl Error { | Error::IoError(_) | Error::Persist(_) | Error::FeatureNotEnabled(_) + | Error::RuntimeFeatureToggleError | Error::Anyhow(_) => true, Error::CreateBatch(_) | Error::CorruptedTaskQueue @@ -232,6 +235,7 @@ impl ErrorCode for Error { Error::IoError(e) => e.error_code(), Error::Persist(e) => e.error_code(), Error::FeatureNotEnabled(_) => Code::FeatureNotEnabled, + Error::RuntimeFeatureToggleError => Code::Internal, // Irrecoverable errors Error::Anyhow(_) => Code::Internal, diff --git a/index-scheduler/src/features.rs b/index-scheduler/src/features.rs index a9d242619..222e4a443 100644 --- a/index-scheduler/src/features.rs +++ b/index-scheduler/src/features.rs @@ -1,7 +1,10 @@ +use std::sync::{Arc, RwLock}; + use meilisearch_types::features::{InstanceTogglableFeatures, RuntimeTogglableFeatures}; use meilisearch_types::heed::types::{SerdeJson, Str}; -use meilisearch_types::heed::{Database, Env, RoTxn, RwTxn}; +use meilisearch_types::heed::{Database, Env, RwTxn}; +use crate::error::Error::RuntimeFeatureToggleError; use crate::error::FeatureNotEnabledError; use crate::Result; @@ -9,7 +12,8 @@ const EXPERIMENTAL_FEATURES: &str = "experimental-features"; #[derive(Clone)] pub(crate) struct FeatureData { - runtime: Database>, + persisted: Database>, + runtime: Arc>, } #[derive(Debug, Clone, Copy)] @@ -18,8 +22,8 @@ pub struct RoFeatures { } impl RoFeatures { - fn new(txn: RoTxn<'_>, data: &FeatureData) -> Result { - let runtime = data.runtime_features(txn)?; + fn new(data: &FeatureData) -> Result { + let runtime = data.runtime_features()?; Ok(Self { runtime }) } @@ -83,13 +87,18 @@ impl RoFeatures { impl FeatureData { pub fn new(env: &Env, instance_features: InstanceTogglableFeatures) -> Result { let mut wtxn = env.write_txn()?; - let runtime_features = env.create_database(&mut wtxn, Some(EXPERIMENTAL_FEATURES))?; - let default_features = - RuntimeTogglableFeatures { metrics: instance_features.metrics, ..Default::default() }; - runtime_features.put(&mut wtxn, EXPERIMENTAL_FEATURES, &default_features)?; + let runtime_features_db = env.create_database(&mut wtxn, Some(EXPERIMENTAL_FEATURES))?; wtxn.commit()?; - Ok(Self { runtime: runtime_features }) + let txn = env.read_txn()?; + let persisted_features: RuntimeTogglableFeatures = + runtime_features_db.get(&txn, EXPERIMENTAL_FEATURES)?.unwrap_or_default(); + let runtime = Arc::new(RwLock::new(RuntimeTogglableFeatures { + metrics: instance_features.metrics || persisted_features.metrics, + ..persisted_features + })); + + Ok(Self { persisted: runtime_features_db, runtime }) } pub fn put_runtime_features( @@ -97,16 +106,19 @@ impl FeatureData { mut wtxn: RwTxn, features: RuntimeTogglableFeatures, ) -> Result<()> { - self.runtime.put(&mut wtxn, EXPERIMENTAL_FEATURES, &features)?; + self.persisted.put(&mut wtxn, EXPERIMENTAL_FEATURES, &features)?; wtxn.commit()?; + + let mut toggled_features = self.runtime.write().map_err(|_| RuntimeFeatureToggleError)?; + *toggled_features = features; Ok(()) } - fn runtime_features(&self, txn: RoTxn) -> Result { - Ok(self.runtime.get(&txn, EXPERIMENTAL_FEATURES)?.unwrap_or_default()) + fn runtime_features(&self) -> Result { + Ok(*self.runtime.read().map_err(|_| RuntimeFeatureToggleError)?) } - pub fn features(&self, txn: RoTxn) -> Result { - RoFeatures::new(txn, self) + pub fn features(&self) -> Result { + RoFeatures::new(self) } } diff --git a/index-scheduler/src/lib.rs b/index-scheduler/src/lib.rs index 0b3a5d58a..b2a263355 100644 --- a/index-scheduler/src/lib.rs +++ b/index-scheduler/src/lib.rs @@ -1300,8 +1300,7 @@ impl IndexScheduler { } pub fn features(&self) -> Result { - let rtxn = self.read_txn()?; - self.features.features(rtxn) + self.features.features() } pub fn put_runtime_features(&self, features: RuntimeTogglableFeatures) -> Result<()> { diff --git a/meilisearch/tests/features/mod.rs b/meilisearch/tests/features/mod.rs index 8ac73c097..abb006ac8 100644 --- a/meilisearch/tests/features/mod.rs +++ b/meilisearch/tests/features/mod.rs @@ -126,6 +126,14 @@ async fn experimental_feature_metrics() { let (response, code) = server.get_metrics().await; meili_snap::snapshot!(code, @"200 OK"); meili_snap::snapshot!(response, @"null"); + + // startup without flag respects persisted metrics value + let disable_metrics = + Opt { experimental_enable_metrics: false, ..default_settings(dir.path()) }; + let server_no_flag = Server::new_with_options(disable_metrics).await.unwrap(); + let (response, code) = server_no_flag.get_metrics().await; + meili_snap::snapshot!(code, @"200 OK"); + meili_snap::snapshot!(response, @"null"); } #[actix_rt::test]