refactor: Consolidate MetaStorageError variants (databendlabs#16760)

Replace multiple error variants with a single `Damaged` variant: - Remove: BytesError, SnapshotError, SledError - Add: Damaged These errors share the same error handling logic, making a single variant more maintainable and semantically clearer.
PlaidCloud · Nov 4, 2024 · dcf7654 · dcf7654
1 parent 0603739
commit dcf7654
Show file tree

Hide file tree

Showing 13 changed files with 26 additions and 112 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/src/meta/embedded/src/meta_embedded.rs b/src/meta/embedded/src/meta_embedded.rs
@@ -72,7 +72,7 @@ impl MetaEmbedded {
     /// Creates a kvapi::KVApi impl with a random and unique name.
     pub async fn new_temp() -> Result<MetaEmbedded, MetaStorageError> {
         let temp_dir =
-            tempfile::tempdir().map_err(|e| MetaStorageError::SledError(AnyError::new(&e)))?;
+            tempfile::tempdir().map_err(|e| MetaStorageError::Damaged(AnyError::new(&e)))?;
 
         init_temp_sled_db(temp_dir);
 

diff --git a/src/meta/raft-store/src/ondisk/mod.rs b/src/meta/raft-store/src/ondisk/mod.rs
@@ -26,7 +26,6 @@ use std::fmt::Debug;
 pub use data_version::DataVersion;
 use databend_common_meta_sled_store::sled;
 use databend_common_meta_sled_store::SledTree;
-use databend_common_meta_stoerr::MetaBytesError;
 use databend_common_meta_stoerr::MetaStorageError;
 pub use header::Header;
 use log::info;
@@ -84,16 +83,16 @@ impl OnDisk {
         let ks = tree.key_space::<DataHeader>();
 
         let header = ks.get(&Self::KEY_HEADER.to_string()).map_err(|e| {
-            MetaStorageError::BytesError(MetaBytesError {
-                source: AnyError::error(format!(
+            MetaStorageError::Damaged(
+                AnyError::error(format!(
                     "Unable to read meta-service data version from disk; \
                     Possible reasons: opening future version meta-service with old version binary, \
                     or the on-disk data is damaged. \
                     error: {}",
                     e
                 ))
                 .add_context(|| "open on-disk data"),
-            })
+            )
         })?;
         info!("Loaded header: {:?}", header);
 
@@ -177,7 +176,7 @@ impl OnDisk {
 
                     let last_snapshot = snapshot_store.load_last_snapshot().await.map_err(|e| {
                         let ae = AnyError::new(&e).add_context(|| "load last snapshot");
-                        MetaStorageError::SnapshotError(ae)
+                        MetaStorageError::Damaged(ae)
                     })?;
 
                     if last_snapshot.is_some() {

diff --git a/src/meta/raft-store/src/ondisk/upgrade_to_v003.rs b/src/meta/raft-store/src/ondisk/upgrade_to_v003.rs
@@ -57,7 +57,7 @@ impl OnDisk {
         self.convert_snapshot_v002_to_v003(snapshot_id.clone(), snapshot_data)
             .await
             .map_err(|e| {
-                MetaStorageError::snapshot_error(&e, || {
+                MetaStorageError::damaged(&e, || {
                     format!("convert v002 snapshot to v003 {}", snapshot_id)
                 })
             })?;
@@ -76,7 +76,7 @@ impl OnDisk {
 
         let last_snapshot = loader.load_last_snapshot().await.map_err(|e| {
             let ae = AnyError::new(&e).add_context(|| "load last snapshot");
-            MetaStorageError::SnapshotError(ae)
+            MetaStorageError::Damaged(ae)
         })?;
 
         if last_snapshot.is_some() {
@@ -115,7 +115,7 @@ impl OnDisk {
         let dir = snapshot_config.snapshot_dir();
 
         fs::remove_dir_all(&dir).map_err(|e| {
-            MetaStorageError::snapshot_error(&e, || format!("removing v002 snapshot dir: {}", dir))
+            MetaStorageError::damaged(&e, || format!("removing v002 snapshot dir: {}", dir))
         })?;
         Ok(())
     }

diff --git a/src/meta/raft-store/src/sm_v003/snapshot_store_v002.rs b/src/meta/raft-store/src/sm_v003/snapshot_store_v002.rs
@@ -93,7 +93,7 @@ impl From<SnapshotStoreError> for StorageError {
 
 impl From<SnapshotStoreError> for MetaStorageError {
     fn from(value: SnapshotStoreError) -> Self {
-        MetaStorageError::snapshot_error(&value.source, || {
+        MetaStorageError::damaged(&value.source, || {
             format!("when {}: {}", value.verb, value.context)
         })
     }

diff --git a/src/meta/service/src/meta_service/meta_node.rs b/src/meta/service/src/meta_service/meta_node.rs
@@ -828,7 +828,7 @@ impl MetaNode {
 
     fn get_db_size(&self) -> Result<u64, MetaError> {
         self.sto.db.size_on_disk().map_err(|e| {
-            let se = MetaStorageError::SledError(AnyError::new(&e).add_context(|| "get db_size"));
+            let se = MetaStorageError::Damaged(AnyError::new(&e).add_context(|| "get db_size"));
             MetaError::StorageError(se)
         })
     }

diff --git a/src/meta/service/src/store/store_inner.rs b/src/meta/service/src/store/store_inner.rs
@@ -129,7 +129,7 @@ impl StoreInner {
 
         fn to_startup_err(e: impl std::error::Error + 'static) -> MetaStartupError {
             let ae = AnyError::new(&e);
-            let store_err = MetaStorageError::SnapshotError(ae);
+            let store_err = MetaStorageError::Damaged(ae);
             MetaStartupError::StoreOpenError(store_err)
         }
 
@@ -296,7 +296,7 @@ impl StoreInner {
     pub async fn do_install_snapshot(&self, db: DB) -> Result<(), MetaStorageError> {
         let mut sm = self.state_machine.write().await;
         sm.install_snapshot_v003(db).await.map_err(|e| {
-            MetaStorageError::SnapshotError(
+            MetaStorageError::Damaged(
                 AnyError::new(&e).add_context(|| "replacing state-machine with snapshot"),
             )
         })?;

diff --git a/src/meta/sled-store/src/bytes_error.rs b/src/meta/sled-store/src/bytes_error.rs
@@ -13,7 +13,6 @@
 // limitations under the License.
 
 use anyerror::AnyError;
-use databend_common_meta_stoerr::MetaBytesError;
 use databend_common_meta_stoerr::MetaStorageError;
 
 /// Errors that occur when encode/decode
@@ -47,6 +46,6 @@ impl From<std::string::FromUtf8Error> for SledBytesError {
 // TODO: remove this: after refactoring, sled should not use MetaStorageError directly.
 impl From<SledBytesError> for MetaStorageError {
     fn from(e: SledBytesError) -> Self {
-        MetaStorageError::BytesError(MetaBytesError::new(&e))
+        MetaStorageError::Damaged(AnyError::new(&e))
     }
 }
diff --git a/src/meta/sled-store/src/sled_tree.rs b/src/meta/sled-store/src/sled_tree.rs
@@ -188,16 +188,10 @@ impl SledTree {
                     warn!("txn error: {:?}", meta_sto_err);
 
                     match &meta_sto_err {
-                        MetaStorageError::BytesError(_e) => {
-                            Err(ConflictableTransactionError::Abort(meta_sto_err))
-                        }
-                        MetaStorageError::SledError(_e) => {
-                            Err(ConflictableTransactionError::Abort(meta_sto_err))
-                        }
                         MetaStorageError::TransactionConflict => {
                             Err(ConflictableTransactionError::Conflict)
                         }
-                        MetaStorageError::SnapshotError(_e) => {
+                        MetaStorageError::Damaged(_e) => {
                             Err(ConflictableTransactionError::Abort(meta_sto_err))
                         }
                     }
@@ -210,7 +204,7 @@ impl SledTree {
             Err(txn_err) => match txn_err {
                 TransactionError::Abort(meta_sto_err) => Err(meta_sto_err),
                 TransactionError::Storage(sto_err) => {
-                    Err(MetaStorageError::SledError(AnyError::new(&sto_err)))
+                    Err(MetaStorageError::Damaged(AnyError::new(&sto_err)))
                 }
             },
         }

diff --git a/src/meta/stoerr/Cargo.toml b/src/meta/stoerr/Cargo.toml
@@ -13,7 +13,6 @@ test = true
 [dependencies]
 anyerror = { workspace = true }
 databend-common-exception = { workspace = true }
-prost = { workspace = true }
 serde_json = { workspace = true }
 sled = { workspace = true }
 thiserror = { workspace = true }

diff --git a/src/meta/stoerr/src/lib.rs b/src/meta/stoerr/src/lib.rs
@@ -12,8 +12,6 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-pub mod meta_bytes_error;
 mod meta_storage_errors;
 
-pub use meta_bytes_error::MetaBytesError;
 pub use meta_storage_errors::MetaStorageError;
diff --git a/src/meta/stoerr/src/meta_bytes_error.rs b/src/meta/stoerr/src/meta_bytes_error.rs
diff --git a/src/meta/stoerr/src/meta_storage_errors.rs b/src/meta/stoerr/src/meta_storage_errors.rs
@@ -19,22 +19,11 @@ use anyerror::AnyError;
 use databend_common_exception::ErrorCode;
 use sled::transaction::UnabortableTransactionError;
 
-use crate::MetaBytesError;
-
 /// Storage level error that is raised by meta service.
 #[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)]
 pub enum MetaStorageError {
-    /// An error raised when encode/decode data to/from underlying storage.
-    #[error(transparent)]
-    BytesError(MetaBytesError),
-
-    /// An AnyError built from sled::Error.
-    #[error(transparent)]
-    SledError(AnyError),
-
-    /// Error that is related to snapshot
-    #[error(transparent)]
-    SnapshotError(AnyError),
+    #[error("Data damaged: {0}")]
+    Damaged(AnyError),
 
     // TODO(1): remove this error
     /// An internal error that inform txn to retry.
@@ -43,52 +32,44 @@ pub enum MetaStorageError {
 }
 
 impl MetaStorageError {
-    pub fn snapshot_error<D: fmt::Display, F: FnOnce() -> D>(
+    pub fn damaged<D: fmt::Display, F: FnOnce() -> D>(
         error: &(impl std::error::Error + 'static),
         context: F,
     ) -> Self {
-        MetaStorageError::SnapshotError(AnyError::new(error).add_context(context))
+        MetaStorageError::Damaged(AnyError::new(error).add_context(context))
     }
 
     pub fn name(&self) -> &'static str {
         match self {
-            MetaStorageError::BytesError(_) => "BytesError",
-            MetaStorageError::SledError(_) => "SledError",
-            MetaStorageError::SnapshotError(_) => "SnapshotError",
+            MetaStorageError::Damaged(_) => "Damaged",
             MetaStorageError::TransactionConflict => "TransactionConflict",
         }
     }
 }
 
 impl From<std::string::FromUtf8Error> for MetaStorageError {
     fn from(error: std::string::FromUtf8Error) -> Self {
-        MetaStorageError::BytesError(MetaBytesError::new(&error))
+        MetaStorageError::Damaged(AnyError::new(&error))
     }
 }
 
 impl From<serde_json::Error> for MetaStorageError {
     fn from(error: serde_json::Error) -> MetaStorageError {
-        MetaStorageError::BytesError(MetaBytesError::new(&error))
-    }
-}
-
-impl From<MetaBytesError> for MetaStorageError {
-    fn from(error: MetaBytesError) -> Self {
-        MetaStorageError::BytesError(error)
+        MetaStorageError::Damaged(AnyError::new(&error))
     }
 }
 
 impl From<sled::Error> for MetaStorageError {
-    fn from(e: sled::Error) -> MetaStorageError {
-        MetaStorageError::SledError(AnyError::new(&e))
+    fn from(error: sled::Error) -> MetaStorageError {
+        MetaStorageError::Damaged(AnyError::new(&error))
     }
 }
 
 impl From<UnabortableTransactionError> for MetaStorageError {
     fn from(error: UnabortableTransactionError) -> Self {
         match error {
-            UnabortableTransactionError::Storage(e) => {
-                MetaStorageError::SledError(AnyError::new(&e))
+            UnabortableTransactionError::Storage(error) => {
+                MetaStorageError::Damaged(AnyError::new(&error))
             }
             UnabortableTransactionError::Conflict => MetaStorageError::TransactionConflict,
         }