Merge #3281

3281: Merge `--schedule-snapshot` and `--snapshot-interval-sec` options r=dureuill a=dureuill

# Pull Request

## Related issue
Fixes #3131

## What does this PR do?
- Removes `--snapshot-interval-sec`
- `--schedule-snapshot` now accepts an optional integer value specifying the interval in seconds
- The config file no longer has a snapshot_interval_sec key.  Instead, the schedule_snapshot key now additionally accepts an integer value specifying the interval in seconds
- The env variable MEILI_SNAPSHOT_INTERVAL no longer exists
- The env variable MEILI_SCHEDULE_SNAPSHOT is always specified to the interval of the snapshot in seconds when defined. If snapshots are disabled the variable is undefined.

---

Relevant part of the `--help`

<img width="885" alt="Capture d’écran 2022-12-27 à 18 22 32" src="https://user-images.githubusercontent.com/41078892/209700626-1a1292c1-14e3-45b6-8265-e0adbd76ecf1.png">

---

### Tests

| `schedule_snapshot` in config.toml | `--schedule-snapshot` flag on CLI | `MEILI_SCHEDULE_SNAPSHOT` | `opt.schedule_snapshot` |
|--|--|--|--|
| missing | missing | missing | `Disabled`
| `false` | missing | missing | `Disabled`
| `true` | missing | missing | `Enabled(86400)`
| `1234` | missing | missing | `Enabled(1234)`
| missing | `--schedule-snapshot` | missing | `Enabled(86400)`
| `false` | `--schedule-snapshot` | missing | `Enabled(86400)` 
| missing | `--schedule-snapshot 2345` | missing | `Enabled(2345)`
| `false` | `--schedule-snapshot 2345` | missing | `Enabled(2345)`
| `true` | `--schedule-snapshot 2345` | missing | `Enabled(2345)`
| `1234` | `--schedule-snapshot 2345` | missing | `Enabled(2345)`
| `false` | `--schedule-snapshot 2345` | 3456 | `Enabled(2345)`
| `false` | `--schedule-snapshot` | 3456 | **`Enabled(86400)`**
| `1234` | missing | 3456 | `Enabled(3456)`
| `false` | missing | 3456 | `Enabled(3456)`


## PR checklist
Please check if your PR fulfills the following requirements:
- [x] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
- [x] Have you read the contributing guidelines?
- [x] Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!


Co-authored-by: Louis Dureuil <louis@meilisearch.com>

config.toml

@@ -73,17 +73,15 @@ ignore_dump_if_db_exists = false
 #################
 
 schedule_snapshot = false
-# Activates scheduled snapshots when provided.
+# Enables scheduled snapshots when true, disable when false (the default).
+# If the value is given as an integer, then enables the scheduled snapshot with the passed value as the interval
+# between each snapshot, in seconds.
 # https://docs.meilisearch.com/learn/configuration/instance_options.html#schedule-snapshot-creation
 
 snapshot_dir = "snapshots/"
 # Sets the directory where Meilisearch will store snapshots.
 # https://docs.meilisearch.com/learn/configuration/instance_options.html#snapshot-destination
 
-snapshot_interval_sec = 86400
-# Defines the interval between each snapshot. Value must be given in seconds.
-# https://docs.meilisearch.com/learn/configuration/instance_options.html#snapshot-interval
-
 # import_snapshot = "./path/to/my/snapshot"
 # Launches Meilisearch after importing a previously-generated snapshot at the given filepath.
 # https://docs.meilisearch.com/learn/configuration/instance_options.html#import-snapshot

meilisearch/src/analytics/segment_analytics.rs

@@ -25,7 +25,7 @@ use uuid::Uuid;
 
 use super::{config_user_id_path, DocumentDeletionKind, MEILISEARCH_CONFIG_PATH};
 use crate::analytics::Analytics;
-use crate::option::{default_http_addr, IndexerOpts, MaxMemory, MaxThreads};
+use crate::option::{default_http_addr, IndexerOpts, MaxMemory, MaxThreads, ScheduleSnapshot};
 use crate::routes::indexes::documents::UpdateDocumentsQuery;
 use crate::routes::tasks::TasksFilterQueryRaw;
 use crate::routes::{create_all_stats, Stats};
@@ -220,9 +220,8 @@ struct Infos {
     ignore_missing_dump: bool,
     ignore_dump_if_db_exists: bool,
     import_snapshot: bool,
-    schedule_snapshot: bool,
+    schedule_snapshot: Option<u64>,
     snapshot_dir: bool,
-    snapshot_interval_sec: u64,
     ignore_missing_snapshot: bool,
     ignore_snapshot_if_db_exists: bool,
     http_addr: bool,
@@ -267,7 +266,6 @@ impl From<Opt> for Infos {
             ignore_snapshot_if_db_exists,
             snapshot_dir,
             schedule_snapshot,
-            snapshot_interval_sec,
             import_dump,
             ignore_missing_dump,
             ignore_dump_if_db_exists,
@@ -280,6 +278,11 @@ impl From<Opt> for Infos {
                 no_analytics: _,
         } = options;
 
+        let schedule_snapshot = match schedule_snapshot {
+            ScheduleSnapshot::Disabled => None,
+            ScheduleSnapshot::Enabled(interval) => Some(interval),
+        };
+
         let IndexerOpts {
             log_every_n: _,
             max_nb_chunks: _,
@@ -299,7 +302,6 @@ impl From<Opt> for Infos {
             import_snapshot: import_snapshot.is_some(),
             schedule_snapshot,
             snapshot_dir: snapshot_dir != PathBuf::from("snapshots/"),
-            snapshot_interval_sec,
             ignore_missing_snapshot,
             ignore_snapshot_if_db_exists,
             http_addr: http_addr != default_http_addr(),

meilisearch/src/lib.rs

@@ -41,6 +41,7 @@ use meilisearch_types::tasks::KindWithContent;
 use meilisearch_types::versioning::{check_version_file, create_version_file};
 use meilisearch_types::{compression, milli, VERSION_FILE_NAME};
 pub use option::Opt;
+use option::ScheduleSnapshot;
 
 use crate::error::MeilisearchHttpError;
 
@@ -169,8 +170,8 @@ pub fn setup_meilisearch(opt: &Opt) -> anyhow::Result<(Arc<IndexScheduler>, Auth
 
     // We create a loop in a thread that registers snapshotCreation tasks
     let index_scheduler = Arc::new(index_scheduler);
-    if opt.schedule_snapshot {
-        let snapshot_delay = Duration::from_secs(opt.snapshot_interval_sec);
+    if let ScheduleSnapshot::Enabled(snapshot_delay) = opt.schedule_snapshot {
+        let snapshot_delay = Duration::from_secs(snapshot_delay);
         let index_scheduler = index_scheduler.clone();
         thread::Builder::new()
             .name(String::from("register-snapshot-tasks"))

meilisearch/src/option.rs

@@ -44,7 +44,6 @@ const MEILI_IGNORE_MISSING_SNAPSHOT: &str = "MEILI_IGNORE_MISSING_SNAPSHOT";
 const MEILI_IGNORE_SNAPSHOT_IF_DB_EXISTS: &str = "MEILI_IGNORE_SNAPSHOT_IF_DB_EXISTS";
 const MEILI_SNAPSHOT_DIR: &str = "MEILI_SNAPSHOT_DIR";
 const MEILI_SCHEDULE_SNAPSHOT: &str = "MEILI_SCHEDULE_SNAPSHOT";
-const MEILI_SNAPSHOT_INTERVAL_SEC: &str = "MEILI_SNAPSHOT_INTERVAL_SEC";
 const MEILI_IMPORT_DUMP: &str = "MEILI_IMPORT_DUMP";
 const MEILI_IGNORE_MISSING_DUMP: &str = "MEILI_IGNORE_MISSING_DUMP";
 const MEILI_IGNORE_DUMP_IF_DB_EXISTS: &str = "MEILI_IGNORE_DUMP_IF_DB_EXISTS";
@@ -63,6 +62,7 @@ const DEFAULT_MAX_TASK_DB_SIZE: &str = "100 GiB";
 const DEFAULT_HTTP_PAYLOAD_SIZE_LIMIT: &str = "100 MB";
 const DEFAULT_SNAPSHOT_DIR: &str = "snapshots/";
 const DEFAULT_SNAPSHOT_INTERVAL_SEC: u64 = 86400;
+const DEFAULT_SNAPSHOT_INTERVAL_SEC_STR: &str = "86400";
 const DEFAULT_DUMP_DIR: &str = "dumps/";
 
 const MEILI_MAX_INDEXING_MEMORY: &str = "MEILI_MAX_INDEXING_MEMORY";
@@ -241,14 +241,11 @@ pub struct Opt {
     pub snapshot_dir: PathBuf,
 
     /// Activates scheduled snapshots when provided. Snapshots are disabled by default.
-    #[clap(long, env = MEILI_SCHEDULE_SNAPSHOT)]
-    #[serde(default)]
-    pub schedule_snapshot: bool,
-
-    /// Defines the interval between each snapshot. Value must be given in seconds.
-    #[clap(long, env = MEILI_SNAPSHOT_INTERVAL_SEC, default_value_t = default_snapshot_interval_sec())]
-    #[serde(default = "default_snapshot_interval_sec")]
-    pub snapshot_interval_sec: u64,
+    ///
+    /// When provided with a value, defines the interval between each snapshot, in seconds.
+    #[clap(long,env = MEILI_SCHEDULE_SNAPSHOT, num_args(0..=1), value_parser=parse_schedule_snapshot, default_value_t, default_missing_value=default_snapshot_interval_sec(),  value_name = "SNAPSHOT_INTERVAL_SEC")]
+    #[serde(default, deserialize_with = "schedule_snapshot_deserialize")]
+    pub schedule_snapshot: ScheduleSnapshot,
 
     /// Imports the dump file located at the specified path. Path must point to a `.dump` file.
     /// If a database already exists, Meilisearch will throw an error and abort launch.
@@ -376,7 +373,6 @@ impl Opt {
             ssl_tickets,
             snapshot_dir,
             schedule_snapshot,
-            snapshot_interval_sec,
             dump_dir,
             log_level,
             indexer_options,
@@ -425,11 +421,10 @@ impl Opt {
         export_to_env_if_not_present(MEILI_SSL_RESUMPTION, ssl_resumption.to_string());
         export_to_env_if_not_present(MEILI_SSL_TICKETS, ssl_tickets.to_string());
         export_to_env_if_not_present(MEILI_SNAPSHOT_DIR, snapshot_dir);
-        export_to_env_if_not_present(MEILI_SCHEDULE_SNAPSHOT, schedule_snapshot.to_string());
-        export_to_env_if_not_present(
-            MEILI_SNAPSHOT_INTERVAL_SEC,
-            snapshot_interval_sec.to_string(),
-        );
+        if let Some(snapshot_interval) = schedule_snapshot_to_env(schedule_snapshot) {
+            export_to_env_if_not_present(MEILI_SCHEDULE_SNAPSHOT, snapshot_interval)
+        }
+
         export_to_env_if_not_present(MEILI_DUMP_DIR, dump_dir);
         export_to_env_if_not_present(MEILI_LOG_LEVEL, log_level.to_string());
         #[cfg(feature = "metrics")]
@@ -744,8 +739,8 @@ fn default_snapshot_dir() -> PathBuf {
     PathBuf::from(DEFAULT_SNAPSHOT_DIR)
 }
 
-fn default_snapshot_interval_sec() -> u64 {
-    DEFAULT_SNAPSHOT_INTERVAL_SEC
+fn default_snapshot_interval_sec() -> &'static str {
+    DEFAULT_SNAPSHOT_INTERVAL_SEC_STR
 }
 
 fn default_dump_dir() -> PathBuf {
@@ -756,6 +751,102 @@ fn default_log_every_n() -> usize {
     DEFAULT_LOG_EVERY_N
 }
 
+/// Indicates if a snapshot was scheduled, and if yes with which interval.
+#[derive(Debug, Default, Copy, Clone, Deserialize, Serialize)]
+pub enum ScheduleSnapshot {
+    /// Scheduled snapshots are disabled.
+    #[default]
+    Disabled,
+    /// Snapshots are scheduled at the specified interval, in seconds.
+    Enabled(u64),
+}
+
+impl Display for ScheduleSnapshot {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            ScheduleSnapshot::Disabled => write!(f, ""),
+            ScheduleSnapshot::Enabled(value) => write!(f, "{}", value),
+        }
+    }
+}
+
+impl FromStr for ScheduleSnapshot {
+    type Err = ParseIntError;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        Ok(match s {
+            "" => ScheduleSnapshot::Disabled,
+            s => ScheduleSnapshot::Enabled(s.parse()?),
+        })
+    }
+}
+
+fn parse_schedule_snapshot(s: &str) -> Result<ScheduleSnapshot, ParseIntError> {
+    Ok(if s.is_empty() { ScheduleSnapshot::Disabled } else { ScheduleSnapshot::from_str(s)? })
+}
+
+fn schedule_snapshot_to_env(schedule_snapshot: ScheduleSnapshot) -> Option<String> {
+    match schedule_snapshot {
+        ScheduleSnapshot::Enabled(snapshot_delay) => Some(snapshot_delay.to_string()),
+        _ => None,
+    }
+}
+
+fn schedule_snapshot_deserialize<'de, D>(deserializer: D) -> Result<ScheduleSnapshot, D::Error>
+where
+    D: serde::Deserializer<'de>,
+{
+    struct BoolOrInt;
+
+    impl<'de> serde::de::Visitor<'de> for BoolOrInt {
+        type Value = ScheduleSnapshot;
+
+        fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
+            formatter.write_str("integer or boolean")
+        }
+
+        fn visit_bool<E>(self, value: bool) -> Result<Self::Value, E>
+        where
+            E: serde::de::Error,
+        {
+            Ok(if value {
+                ScheduleSnapshot::Enabled(DEFAULT_SNAPSHOT_INTERVAL_SEC)
+            } else {
+                ScheduleSnapshot::Disabled
+            })
+        }
+
+        fn visit_i64<E>(self, v: i64) -> Result<Self::Value, E>
+        where
+            E: serde::de::Error,
+        {
+            Ok(ScheduleSnapshot::Enabled(v as u64))
+        }
+
+        fn visit_u64<E>(self, v: u64) -> Result<Self::Value, E>
+        where
+            E: serde::de::Error,
+        {
+            Ok(ScheduleSnapshot::Enabled(v))
+        }
+
+        fn visit_none<E>(self) -> Result<Self::Value, E>
+        where
+            E: serde::de::Error,
+        {
+            Ok(ScheduleSnapshot::Disabled)
+        }
+
+        fn visit_unit<E>(self) -> Result<Self::Value, E>
+        where
+            E: serde::de::Error,
+        {
+            Ok(ScheduleSnapshot::Disabled)
+        }
+    }
+    deserializer.deserialize_any(BoolOrInt)
+}
+
 #[cfg(test)]
 mod test {
 

meilisearch/tests/snapshot/mod.rs

@@ -1,5 +1,6 @@
 use std::time::Duration;
 
+use meilisearch::option::ScheduleSnapshot;
 use meilisearch::Opt;
 use tokio::time::sleep;
 
@@ -36,8 +37,7 @@ async fn perform_snapshot() {
 
     let options = Opt {
         snapshot_dir: snapshot_dir.path().to_owned(),
-        snapshot_interval_sec: 1,
-        schedule_snapshot: true,
+        schedule_snapshot: ScheduleSnapshot::Enabled(1),
         ..default_settings(temp.path())
     };