refactor: remove unused error variants (#4666)

* add python script Signed-off-by: Ruihang Xia <[email protected]> * remove unused errors Signed-off-by: Ruihang Xia <[email protected]> * fix all negative cases Signed-off-by: Ruihang Xia <[email protected]> * setup CI Signed-off-by: Ruihang Xia <[email protected]> * add license header Signed-off-by: Ruihang Xia <[email protected]> --------- Signed-off-by: Ruihang Xia <[email protected]>
GreptimeTeam · Sep 3, 2024 · 93f2026 · 93f2026
1 parent b52e3c6
commit 93f2026
Show file tree

Hide file tree

Showing 39 changed files with 183 additions and 897 deletions.
diff --git a/.github/workflows/develop.yml b/.github/workflows/develop.yml
@@ -616,8 +616,8 @@ jobs:
         with:
           # Shares across multiple jobs
           shared-key: "check-rust-fmt"
-      - name: Run cargo fmt
-        run: cargo fmt --all -- --check
+      - name: Check format
+        run: make fmt-check
 
   clippy:
     name: Clippy

diff --git a/Makefile b/Makefile
@@ -191,6 +191,7 @@ fix-clippy: ## Fix clippy violations.
 .PHONY: fmt-check
 fmt-check: ## Check code format.
 	cargo fmt --all -- --check
+	python3 scripts/check-snafu.py
 
 .PHONY: start-etcd
 start-etcd: ## Start single node etcd for testing purpose.

diff --git a/scripts/check-snafu.py b/scripts/check-snafu.py
@@ -0,0 +1,69 @@
+# Copyright 2023 Greptime Team
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import os
+import re
+
+
+def find_rust_files(directory):
+    error_files = []
+    other_rust_files = []
+    for root, _, files in os.walk(directory):
+        for file in files:
+            if file == "error.rs":
+                error_files.append(os.path.join(root, file))
+            elif file.endswith(".rs"):
+                other_rust_files.append(os.path.join(root, file))
+    return error_files, other_rust_files
+
+
+def extract_branch_names(file_content):
+    pattern = re.compile(r"#\[snafu\(display\([^\)]*\)\)\]\s*(\w+)\s*\{")
+    return pattern.findall(file_content)
+
+
+def check_snafu_in_files(branch_name, rust_files):
+    branch_name_snafu = f"{branch_name}Snafu"
+    for rust_file in rust_files:
+        with open(rust_file, "r") as file:
+            content = file.read()
+            if branch_name_snafu in content:
+                return True
+    return False
+
+
+def main():
+    error_files, other_rust_files = find_rust_files(".")
+    branch_names = []
+
+    for error_file in error_files:
+        with open(error_file, "r") as file:
+            content = file.read()
+            branch_names.extend(extract_branch_names(content))
+
+    unused_snafu = [
+        branch_name
+        for branch_name in branch_names
+        if not check_snafu_in_files(branch_name, other_rust_files)
+    ]
+
+    for name in unused_snafu:
+        print(name)
+
+    if unused_snafu:
+        raise SystemExit(1)
+
+
+if __name__ == "__main__":
+    main()
diff --git a/src/auth/src/error.rs b/src/auth/src/error.rs
@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use common_error::ext::{BoxedError, ErrorExt};
+use common_error::ext::ErrorExt;
 use common_error::status_code::StatusCode;
 use common_macro::stack_trace_debug;
 use snafu::{Location, Snafu};
@@ -38,14 +38,6 @@ pub enum Error {
         location: Location,
     },
 
-    #[snafu(display("Authentication source failure"))]
-    AuthBackend {
-        #[snafu(implicit)]
-        location: Location,
-        #[snafu(source)]
-        source: BoxedError,
-    },
-
     #[snafu(display("User not found, username: {}", username))]
     UserNotFound { username: String },
 
@@ -89,7 +81,6 @@ impl ErrorExt for Error {
             Error::FileWatch { .. } => StatusCode::InvalidArguments,
             Error::InternalState { .. } => StatusCode::Unexpected,
             Error::Io { .. } => StatusCode::StorageUnavailable,
-            Error::AuthBackend { .. } => StatusCode::Internal,
 
             Error::UserNotFound { .. } => StatusCode::UserNotFound,
             Error::UnsupportedPasswordType { .. } => StatusCode::UnsupportedPasswordType,

diff --git a/src/auth/tests/mod.rs b/src/auth/tests/mod.rs
@@ -18,6 +18,7 @@ use std::sync::Arc;
 
 use api::v1::greptime_request::Request;
 use auth::error::Error::InternalState;
+use auth::error::InternalStateSnafu;
 use auth::{PermissionChecker, PermissionCheckerRef, PermissionReq, PermissionResp, UserInfoRef};
 use sql::statements::show::{ShowDatabases, ShowKind};
 use sql::statements::statement::Statement;
@@ -33,9 +34,10 @@ impl PermissionChecker for DummyPermissionChecker {
         match req {
             PermissionReq::GrpcRequest(_) => Ok(PermissionResp::Allow),
             PermissionReq::SqlStatement(_) => Ok(PermissionResp::Reject),
-            _ => Err(InternalState {
+            _ => InternalStateSnafu {
                 msg: "testing".to_string(),
-            }),
+            }
+            .fail(),
         }
     }
 }

diff --git a/src/catalog/src/error.rs b/src/catalog/src/error.rs
@@ -97,13 +97,6 @@ pub enum Error {
         source: table::error::Error,
     },
 
-    #[snafu(display("System catalog is not valid: {}", msg))]
-    SystemCatalog {
-        msg: String,
-        #[snafu(implicit)]
-        location: Location,
-    },
-
     #[snafu(display("Cannot find catalog by name: {}", catalog_name))]
     CatalogNotFound {
         catalog_name: String,
@@ -186,13 +179,6 @@ pub enum Error {
         source: common_query::error::Error,
     },
 
-    #[snafu(display("Failed to perform metasrv operation"))]
-    Metasrv {
-        #[snafu(implicit)]
-        location: Location,
-        source: meta_client::error::Error,
-    },
-
     #[snafu(display("Invalid table info in catalog"))]
     InvalidTableInfoInCatalog {
         #[snafu(implicit)]
@@ -288,8 +274,6 @@ impl ErrorExt for Error {
 
             Error::FlowInfoNotFound { .. } => StatusCode::FlowNotFound,
 
-            Error::SystemCatalog { .. } => StatusCode::StorageUnavailable,
-
             Error::UpgradeWeakCatalogManagerRef { .. } => StatusCode::Internal,
 
             Error::CreateRecordBatch { source, .. } => source.status_code(),
@@ -303,7 +287,6 @@ impl ErrorExt for Error {
 
             Error::CreateTable { source, .. } => source.status_code(),
 
-            Error::Metasrv { source, .. } => source.status_code(),
             Error::DecodePlan { source, .. } => source.status_code(),
             Error::InvalidTableInfoInCatalog { source, .. } => source.status_code(),
 
@@ -338,27 +321,6 @@ mod tests {
 
     use super::*;
 
-    #[test]
-    pub fn test_error_status_code() {
-        assert_eq!(
-            StatusCode::TableAlreadyExists,
-            Error::TableExists {
-                table: "some_table".to_string(),
-                location: Location::generate(),
-            }
-            .status_code()
-        );
-
-        assert_eq!(
-            StatusCode::StorageUnavailable,
-            Error::SystemCatalog {
-                msg: String::default(),
-                location: Location::generate(),
-            }
-            .status_code()
-        );
-    }
-
     #[test]
     pub fn test_errors_to_datafusion_error() {
         let e: DataFusionError = Error::TableExists {

diff --git a/src/catalog/src/kvbackend/client.rs b/src/catalog/src/kvbackend/client.rs
@@ -20,8 +20,8 @@ use std::time::Duration;
 
 use common_error::ext::BoxedError;
 use common_meta::cache_invalidator::KvCacheInvalidator;
-use common_meta::error::Error::{CacheNotGet, GetKvCache};
-use common_meta::error::{CacheNotGetSnafu, Error, ExternalSnafu, Result};
+use common_meta::error::Error::CacheNotGet;
+use common_meta::error::{CacheNotGetSnafu, Error, ExternalSnafu, GetKvCacheSnafu, Result};
 use common_meta::kv_backend::{KvBackend, KvBackendRef, TxnService};
 use common_meta::rpc::store::{
     BatchDeleteRequest, BatchDeleteResponse, BatchGetRequest, BatchGetResponse, BatchPutRequest,
@@ -282,8 +282,11 @@ impl KvBackend for CachedMetaKvBackend {
                 _ => Err(e),
             },
         }
-        .map_err(|e| GetKvCache {
-            err_msg: e.to_string(),
+        .map_err(|e| {
+            GetKvCacheSnafu {
+                err_msg: e.to_string(),
+            }
+            .build()
         });
 
         // "cache.invalidate_key" and "cache.try_get_with_by_ref" are not mutually exclusive. So we need

diff --git a/src/client/src/database.rs b/src/client/src/database.rs
@@ -37,7 +37,8 @@ use tonic::metadata::AsciiMetadataKey;
 use tonic::transport::Channel;
 
 use crate::error::{
-    ConvertFlightDataSnafu, Error, IllegalFlightMessagesSnafu, InvalidAsciiSnafu, ServerSnafu,
+    ConvertFlightDataSnafu, Error, FlightGetSnafu, IllegalFlightMessagesSnafu, InvalidAsciiSnafu,
+    ServerSnafu,
 };
 use crate::{from_grpc_response, Client, Result};
 
@@ -225,16 +226,18 @@ impl Database {
 
         let mut client = self.client.make_flight_client()?;
 
-        let response = client.mut_inner().do_get(request).await.map_err(|e| {
+        let response = client.mut_inner().do_get(request).await.or_else(|e| {
             let tonic_code = e.code();
             let e: Error = e.into();
             let code = e.status_code();
             let msg = e.to_string();
-            let error = Error::FlightGet {
-                tonic_code,
-                addr: client.addr().to_string(),
-                source: BoxedError::new(ServerSnafu { code, msg }.build()),
-            };
+            let error =
+                Err(BoxedError::new(ServerSnafu { code, msg }.build())).with_context(|_| {
+                    FlightGetSnafu {
+                        addr: client.addr().to_string(),
+                        tonic_code,
+                    }
+                });
             error!(
                 "Failed to do Flight get, addr: {}, code: {}, source: {:?}",
                 client.addr(),

diff --git a/src/client/src/error.rs b/src/client/src/error.rs
@@ -39,13 +39,6 @@ pub enum Error {
         source: BoxedError,
     },
 
-    #[snafu(display("Failure occurs during handling request"))]
-    HandleRequest {
-        #[snafu(implicit)]
-        location: Location,
-        source: BoxedError,
-    },
-
     #[snafu(display("Failed to convert FlightData"))]
     ConvertFlightData {
         #[snafu(implicit)]
@@ -116,13 +109,6 @@ pub enum Error {
         location: Location,
     },
 
-    #[snafu(display("Failed to send request with streaming: {}", err_msg))]
-    ClientStreaming {
-        err_msg: String,
-        #[snafu(implicit)]
-        location: Location,
-    },
-
     #[snafu(display("Failed to parse ascii string: {}", value))]
     InvalidAscii {
         value: String,
@@ -138,12 +124,10 @@ impl ErrorExt for Error {
         match self {
             Error::IllegalFlightMessages { .. }
             | Error::MissingField { .. }
-            | Error::IllegalDatabaseResponse { .. }
-            | Error::ClientStreaming { .. } => StatusCode::Internal,
+            | Error::IllegalDatabaseResponse { .. } => StatusCode::Internal,
 
             Error::Server { code, .. } => *code,
             Error::FlightGet { source, .. }
-            | Error::HandleRequest { source, .. }
             | Error::RegionServer { source, .. }
             | Error::FlowServer { source, .. } => source.status_code(),
             Error::CreateChannel { source, .. }

diff --git a/src/client/src/flow.rs b/src/client/src/flow.rs
@@ -16,9 +16,9 @@ use api::v1::flow::{FlowRequest, FlowResponse};
 use api::v1::region::InsertRequests;
 use common_error::ext::BoxedError;
 use common_meta::node_manager::Flownode;
-use snafu::{location, ResultExt};
+use snafu::ResultExt;
 
-use crate::error::Result;
+use crate::error::{FlowServerSnafu, Result};
 use crate::Client;
 
 #[derive(Debug)]
@@ -57,15 +57,10 @@ impl FlowRequester {
         let response = client
             .handle_create_remove(request)
             .await
-            .map_err(|e| {
+            .or_else(|e| {
                 let code = e.code();
                 let err: crate::error::Error = e.into();
-                crate::error::Error::FlowServer {
-                    addr,
-                    code,
-                    source: BoxedError::new(err),
-                    location: location!(),
-                }
+                Err(BoxedError::new(err)).context(FlowServerSnafu { addr, code })
             })?
             .into_inner();
         Ok(response)
@@ -88,15 +83,10 @@ impl FlowRequester {
         let response = client
             .handle_mirror_request(requests)
             .await
-            .map_err(|e| {
+            .or_else(|e| {
                 let code = e.code();
                 let err: crate::error::Error = e.into();
-                crate::error::Error::FlowServer {
-                    addr,
-                    code,
-                    source: BoxedError::new(err),
-                    location: location!(),
-                }
+                Err(BoxedError::new(err)).context(FlowServerSnafu { addr, code })
             })?
             .into_inner();
         Ok(response)

diff --git a/src/client/src/region.rs b/src/client/src/region.rs
@@ -38,8 +38,8 @@ use substrait::{DFLogicalSubstraitConvertor, SubstraitPlan};
 use tokio_stream::StreamExt;
 
 use crate::error::{
-    self, ConvertFlightDataSnafu, IllegalDatabaseResponseSnafu, IllegalFlightMessagesSnafu,
-    MissingFieldSnafu, Result, ServerSnafu,
+    self, ConvertFlightDataSnafu, FlightGetSnafu, IllegalDatabaseResponseSnafu,
+    IllegalFlightMessagesSnafu, MissingFieldSnafu, Result, ServerSnafu,
 };
 use crate::{metrics, Client, Error};
 
@@ -103,11 +103,14 @@ impl RegionRequester {
                 let e: error::Error = e.into();
                 let code = e.status_code();
                 let msg = e.to_string();
-                let error = Error::FlightGet {
-                    tonic_code,
-                    addr: flight_client.addr().to_string(),
-                    source: BoxedError::new(ServerSnafu { code, msg }.build()),
-                };
+                let error = ServerSnafu { code, msg }
+                    .fail::<()>()
+                    .map_err(BoxedError::new)
+                    .with_context(|_| FlightGetSnafu {
+                        tonic_code,
+                        addr: flight_client.addr().to_string(),
+                    })
+                    .unwrap_err();
                 error!(
                     e; "Failed to do Flight get, addr: {}, code: {}",
                     flight_client.addr(),