From cf8dad1ca0f182a427d9f9f47774c89605722712 Mon Sep 17 00:00:00 2001
From: Louis Dureuil <louis@meilisearch.com>
Date: Mon, 23 Oct 2023 10:38:56 +0200
Subject: [PATCH] index_scheduler.features() is no longer fallible
---
index-scheduler/src/batch.rs | 2 +-
index-scheduler/src/error.rs | 8 -------
index-scheduler/src/features.rs | 21 ++++++++++++-------
index-scheduler/src/lib.rs | 10 ++-------
meilisearch/src/middleware.rs | 11 +---------
meilisearch/src/routes/features.rs | 8 +++----
.../src/routes/indexes/facet_search.rs | 2 +-
meilisearch/src/routes/indexes/search.rs | 4 ++--
meilisearch/src/routes/metrics.rs | 2 +-
meilisearch/src/routes/multi_search.rs | 2 +-
10 files changed, 26 insertions(+), 44 deletions(-)
diff --git a/index-scheduler/src/batch.rs b/index-scheduler/src/batch.rs
index 48eae0063..3e2cc4281 100644
--- a/index-scheduler/src/batch.rs
+++ b/index-scheduler/src/batch.rs
@@ -896,7 +896,7 @@ impl IndexScheduler {
})?;
// 4. Dump experimental feature settings
- let features = self.features()?.runtime_features();
+ let features = self.features().runtime_features();
dump.create_experimental_features(features)?;
let dump_uid = started_at.format(format_description!(
diff --git a/index-scheduler/src/error.rs b/index-scheduler/src/error.rs
index d901129d2..ddc6960f7 100644
--- a/index-scheduler/src/error.rs
+++ b/index-scheduler/src/error.rs
@@ -113,8 +113,6 @@ pub enum Error {
Dump(#[from] dump::Error),
#[error(transparent)]
Heed(#[from] heed::Error),
- #[error("Unable to record metrics for request.")]
- CannotRecordMetrics,
#[error(transparent)]
Milli(#[from] milli::Error),
#[error("An unexpected crash occurred when processing the task.")]
@@ -127,8 +125,6 @@ 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),
@@ -181,14 +177,12 @@ impl Error {
| Error::TaskCancelationWithEmptyQuery
| Error::Dump(_)
| Error::Heed(_)
- | Error::CannotRecordMetrics
| Error::Milli(_)
| Error::ProcessBatchPanicked
| Error::FileStore(_)
| Error::IoError(_)
| Error::Persist(_)
| Error::FeatureNotEnabled(_)
- | Error::RuntimeFeatureToggleError
| Error::Anyhow(_) => true,
Error::CreateBatch(_)
| Error::CorruptedTaskQueue
@@ -229,13 +223,11 @@ impl ErrorCode for Error {
Error::Milli(e) => e.error_code(),
Error::ProcessBatchPanicked => Code::Internal,
Error::Heed(e) => e.error_code(),
- Error::CannotRecordMetrics => Code::Internal,
Error::HeedTransaction(e) => e.error_code(),
Error::FileStore(e) => e.error_code(),
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 222e4a443..1db27bcd5 100644
--- a/index-scheduler/src/features.rs
+++ b/index-scheduler/src/features.rs
@@ -4,7 +4,6 @@ use meilisearch_types::features::{InstanceTogglableFeatures, RuntimeTogglableFea
use meilisearch_types::heed::types::{SerdeJson, Str};
use meilisearch_types::heed::{Database, Env, RwTxn};
-use crate::error::Error::RuntimeFeatureToggleError;
use crate::error::FeatureNotEnabledError;
use crate::Result;
@@ -22,9 +21,9 @@ pub struct RoFeatures {
}
impl RoFeatures {
- fn new(data: &FeatureData) -> Result<Self> {
- let runtime = data.runtime_features()?;
- Ok(Self { runtime })
+ fn new(data: &FeatureData) -> Self {
+ let runtime = data.runtime_features();
+ Self { runtime }
}
pub fn runtime_features(&self) -> RuntimeTogglableFeatures {
@@ -109,16 +108,22 @@ impl FeatureData {
self.persisted.put(&mut wtxn, EXPERIMENTAL_FEATURES, &features)?;
wtxn.commit()?;
- let mut toggled_features = self.runtime.write().map_err(|_| RuntimeFeatureToggleError)?;
+ // safe to unwrap, the lock will only fail if:
+ // 1. requested by the same thread concurrently -> it is called and released in methods that don't call each other
+ // 2. there's a panic while the thread is held -> it is only used for an assignment here.
+ let mut toggled_features = self.runtime.write().unwrap();
*toggled_features = features;
Ok(())
}
- fn runtime_features(&self) -> Result<RuntimeTogglableFeatures> {
- Ok(*self.runtime.read().map_err(|_| RuntimeFeatureToggleError)?)
+ fn runtime_features(&self) -> RuntimeTogglableFeatures {
+ // sound to unwrap, the lock will only fail if:
+ // 1. requested by the same thread concurrently -> it is called and released in methods that don't call each other
+ // 2. there's a panic while the thread is held -> it is only used for copying the data here
+ *self.runtime.read().unwrap()
}
- pub fn features(&self) -> Result<RoFeatures> {
+ pub fn features(&self) -> RoFeatures {
RoFeatures::new(self)
}
}
diff --git a/index-scheduler/src/lib.rs b/index-scheduler/src/lib.rs
index b2a263355..43ac2355c 100644
--- a/index-scheduler/src/lib.rs
+++ b/index-scheduler/src/lib.rs
@@ -579,13 +579,7 @@ impl IndexScheduler {
run.wake_up.wait();
loop {
- let puffin_enabled = match run.features() {
- Ok(features) => features.check_puffin().is_ok(),
- Err(e) => {
- log::error!("{e}");
- continue;
- }
- };
+ let puffin_enabled = run.features().check_puffin().is_ok();
puffin::set_scopes_on(puffin_enabled);
puffin::GlobalProfiler::lock().new_frame();
@@ -1299,7 +1293,7 @@ impl IndexScheduler {
Ok(IndexStats { is_indexing, inner_stats: index_stats })
}
- pub fn features(&self) -> Result<RoFeatures> {
+ pub fn features(&self) -> RoFeatures {
self.features.features()
}
diff --git a/meilisearch/src/middleware.rs b/meilisearch/src/middleware.rs
index 7f7429186..5b87dee34 100644
--- a/meilisearch/src/middleware.rs
+++ b/meilisearch/src/middleware.rs
@@ -6,9 +6,7 @@ use actix_web::dev::{self, Service, ServiceRequest, ServiceResponse, Transform};
use actix_web::web::Data;
use actix_web::Error;
use futures_util::future::LocalBoxFuture;
-use index_scheduler::Error::CannotRecordMetrics;
use index_scheduler::IndexScheduler;
-use meilisearch_types::error::ResponseError;
use prometheus::HistogramTimer;
pub struct RouteMetrics;
@@ -55,14 +53,7 @@ where
// calling unwrap here is safe because index scheduler is added to app data while creating actix app.
// also, the tests will fail if this is not present.
let index_scheduler = req.app_data::<Data<IndexScheduler>>().unwrap();
- let features = match index_scheduler.features() {
- Ok(features) => features,
- Err(_e) => {
- return Box::pin(
- async move { Err(ResponseError::from(CannotRecordMetrics).into()) },
- );
- }
- };
+ let features = index_scheduler.features();
if features.check_metrics().is_ok() {
let request_path = req.path();
diff --git a/meilisearch/src/routes/features.rs b/meilisearch/src/routes/features.rs
index 4437f602d..e7fd8de22 100644
--- a/meilisearch/src/routes/features.rs
+++ b/meilisearch/src/routes/features.rs
@@ -29,12 +29,12 @@ async fn get_features(
>,
req: HttpRequest,
analytics: Data<dyn Analytics>,
-) -> Result<HttpResponse, ResponseError> {
- let features = index_scheduler.features()?;
+) -> HttpResponse {
+ let features = index_scheduler.features();
analytics.publish("Experimental features Seen".to_string(), json!(null), Some(&req));
debug!("returns: {:?}", features.runtime_features());
- Ok(HttpResponse::Ok().json(features.runtime_features()))
+ HttpResponse::Ok().json(features.runtime_features())
}
#[derive(Debug, Deserr)]
@@ -59,7 +59,7 @@ async fn patch_features(
req: HttpRequest,
analytics: Data<dyn Analytics>,
) -> Result<HttpResponse, ResponseError> {
- let features = index_scheduler.features()?;
+ let features = index_scheduler.features();
let old_features = features.runtime_features();
let new_features = meilisearch_types::features::RuntimeTogglableFeatures {
diff --git a/meilisearch/src/routes/indexes/facet_search.rs b/meilisearch/src/routes/indexes/facet_search.rs
index 831c45a85..142a424c0 100644
--- a/meilisearch/src/routes/indexes/facet_search.rs
+++ b/meilisearch/src/routes/indexes/facet_search.rs
@@ -68,7 +68,7 @@ pub async fn search(
}
let index = index_scheduler.index(&index_uid)?;
- let features = index_scheduler.features()?;
+ let features = index_scheduler.features();
let search_result = tokio::task::spawn_blocking(move || {
perform_facet_search(&index, search_query, facet_query, facet_name, features)
})
diff --git a/meilisearch/src/routes/indexes/search.rs b/meilisearch/src/routes/indexes/search.rs
index 3eabf61f3..5a0a9e92b 100644
--- a/meilisearch/src/routes/indexes/search.rs
+++ b/meilisearch/src/routes/indexes/search.rs
@@ -157,7 +157,7 @@ pub async fn search_with_url_query(
let mut aggregate = SearchAggregator::from_query(&query, &req);
let index = index_scheduler.index(&index_uid)?;
- let features = index_scheduler.features()?;
+ let features = index_scheduler.features();
let search_result =
tokio::task::spawn_blocking(move || perform_search(&index, query, features)).await?;
if let Ok(ref search_result) = search_result {
@@ -192,7 +192,7 @@ pub async fn search_with_post(
let index = index_scheduler.index(&index_uid)?;
- let features = index_scheduler.features()?;
+ let features = index_scheduler.features();
let search_result =
tokio::task::spawn_blocking(move || perform_search(&index, query, features)).await?;
if let Ok(ref search_result) = search_result {
diff --git a/meilisearch/src/routes/metrics.rs b/meilisearch/src/routes/metrics.rs
index af1f2b536..7a13a758f 100644
--- a/meilisearch/src/routes/metrics.rs
+++ b/meilisearch/src/routes/metrics.rs
@@ -19,7 +19,7 @@ pub async fn get_metrics(
index_scheduler: GuardedData<ActionPolicy<{ actions::METRICS_GET }>, Data<IndexScheduler>>,
auth_controller: Data<AuthController>,
) -> Result<HttpResponse, ResponseError> {
- index_scheduler.features()?.check_metrics()?;
+ index_scheduler.features().check_metrics()?;
let auth_filters = index_scheduler.filters();
if !auth_filters.all_indexes_authorized() {
let mut error = ResponseError::from(AuthenticationError::InvalidToken);
diff --git a/meilisearch/src/routes/multi_search.rs b/meilisearch/src/routes/multi_search.rs
index e257af1cf..3a028022a 100644
--- a/meilisearch/src/routes/multi_search.rs
+++ b/meilisearch/src/routes/multi_search.rs
@@ -41,7 +41,7 @@ pub async fn multi_search_with_post(
let queries = params.into_inner().queries;
let mut multi_aggregate = MultiSearchAggregator::from_queries(&queries, &req);
- let features = index_scheduler.features()?;
+ let features = index_scheduler.features();
// Explicitly expect a `(ResponseError, usize)` for the error type rather than `ResponseError` only,
// so that `?` doesn't work if it doesn't use `with_index`, ensuring that it is not forgotten in case of code