refactor: improve error code handling in status code conversion (#5851)

* refactor: improve error code handling in status code conversion

* chore: apply suggestions from CR

* fix: only hanlde client side thrown error

* feat: introduce `DeadlineExceeded`

* fix: exclude Code::Unknown from retry conditions
This commit is contained in:
Weny Xu
2025-04-09 15:58:23 +08:00
committed by GitHub
parent 6c66ec3ffc
commit 746b4e2369
9 changed files with 53 additions and 32 deletions

View File

@@ -15,7 +15,7 @@
use std::any::Any;
use common_error::ext::{BoxedError, ErrorExt};
use common_error::status_code::StatusCode;
use common_error::status_code::{convert_tonic_code_to_status_code, StatusCode};
use common_error::{GREPTIME_DB_HEADER_ERROR_CODE, GREPTIME_DB_HEADER_ERROR_MSG};
use common_macro::stack_trace_debug;
use snafu::{location, Location, Snafu};
@@ -152,15 +152,15 @@ impl From<Status> for Error {
.and_then(|v| String::from_utf8(v.as_bytes().to_vec()).ok())
}
let code = get_metadata_value(&e, GREPTIME_DB_HEADER_ERROR_CODE)
.and_then(|s| {
if let Ok(code) = s.parse::<u32>() {
StatusCode::from_u32(code)
} else {
None
}
})
.unwrap_or(StatusCode::Unknown);
let code = get_metadata_value(&e, GREPTIME_DB_HEADER_ERROR_CODE).and_then(|s| {
if let Ok(code) = s.parse::<u32>() {
StatusCode::from_u32(code)
} else {
None
}
});
let tonic_code = e.code();
let code = code.unwrap_or_else(|| convert_tonic_code_to_status_code(tonic_code));
let msg = get_metadata_value(&e, GREPTIME_DB_HEADER_ERROR_MSG)
.unwrap_or_else(|| e.message().to_string());
@@ -187,9 +187,6 @@ impl Error {
} | Self::RegionServer {
code: Code::Unavailable,
..
} | Self::RegionServer {
code: Code::Unknown,
..
}
)
}

View File

@@ -201,12 +201,11 @@ impl RegionRequester {
.await
.map_err(|e| {
let code = e.code();
let err: error::Error = e.into();
// Uses `Error::RegionServer` instead of `Error::Server`
error::Error::RegionServer {
addr,
code,
source: BoxedError::new(err),
source: BoxedError::new(error::Error::from(e)),
location: location!(),
}
})?

View File

@@ -34,12 +34,14 @@ pub enum StatusCode {
Internal = 1003,
/// Invalid arguments.
InvalidArguments = 1004,
/// The task is cancelled.
/// The task is cancelled (typically caller-side).
Cancelled = 1005,
/// Illegal state, can be exposed to users.
IllegalState = 1006,
/// Caused by some error originated from external system.
External = 1007,
/// The request is deadline exceeded (typically server-side).
DeadlineExceeded = 1008,
// ====== End of common status code ================
// ====== Begin of SQL related status code =========
@@ -142,6 +144,7 @@ impl StatusCode {
| StatusCode::Unexpected
| StatusCode::InvalidArguments
| StatusCode::Cancelled
| StatusCode::DeadlineExceeded
| StatusCode::InvalidSyntax
| StatusCode::DatabaseAlreadyExists
| StatusCode::PlanQuery
@@ -177,6 +180,7 @@ impl StatusCode {
| StatusCode::Unexpected
| StatusCode::Internal
| StatusCode::Cancelled
| StatusCode::DeadlineExceeded
| StatusCode::IllegalState
| StatusCode::EngineExecuteQuery
| StatusCode::StorageUnavailable
@@ -272,6 +276,7 @@ pub fn status_to_tonic_code(status_code: StatusCode) -> Code {
Code::InvalidArgument
}
StatusCode::Cancelled => Code::Cancelled,
StatusCode::DeadlineExceeded => Code::DeadlineExceeded,
StatusCode::TableAlreadyExists
| StatusCode::TableColumnExists
| StatusCode::RegionAlreadyExists
@@ -299,6 +304,15 @@ pub fn status_to_tonic_code(status_code: StatusCode) -> Code {
}
}
/// Converts tonic [Code] to [StatusCode].
pub fn convert_tonic_code_to_status_code(code: Code) -> StatusCode {
match code {
Code::Cancelled => StatusCode::Cancelled,
Code::DeadlineExceeded => StatusCode::DeadlineExceeded,
_ => StatusCode::Internal,
}
}
#[cfg(test)]
mod tests {
use strum::IntoEnumIterator;

View File

@@ -30,7 +30,7 @@ use common_meta::rpc::store::{
BatchPutResponse, DeleteRangeRequest, DeleteRangeResponse, PutRequest, PutResponse,
RangeRequest, RangeResponse,
};
use common_telemetry::{info, warn};
use common_telemetry::{error, info, warn};
use snafu::{ensure, ResultExt};
use tokio::sync::RwLock;
use tonic::codec::CompressionEncoding;
@@ -237,6 +237,7 @@ impl Inner {
times += 1;
continue;
} else {
error!("An error occurred in gRPC, status: {status}");
return Err(Error::from(status));
}
}

View File

@@ -24,7 +24,7 @@ use api::v1::meta::{
};
use common_grpc::channel_manager::ChannelManager;
use common_telemetry::tracing_context::TracingContext;
use common_telemetry::{info, warn};
use common_telemetry::{error, info, warn};
use snafu::{ensure, ResultExt};
use tokio::sync::RwLock;
use tonic::codec::CompressionEncoding;
@@ -199,6 +199,7 @@ impl Inner {
times += 1;
continue;
} else {
error!("An error occurred in gRPC, status: {status}");
return Err(error::Error::from(status));
}
}

View File

@@ -13,10 +13,10 @@
// limitations under the License.
use common_error::ext::ErrorExt;
use common_error::status_code::StatusCode;
use common_error::status_code::{convert_tonic_code_to_status_code, StatusCode};
use common_error::{GREPTIME_DB_HEADER_ERROR_CODE, GREPTIME_DB_HEADER_ERROR_MSG};
use common_macro::stack_trace_debug;
use snafu::{Location, Snafu};
use snafu::{location, Location, Snafu};
use tonic::Status;
#[derive(Snafu)]
@@ -35,6 +35,8 @@ pub enum Error {
code: StatusCode,
msg: String,
tonic_code: tonic::Code,
#[snafu(implicit)]
location: Location,
},
#[snafu(display("No leader, should ask leader first"))]
@@ -168,15 +170,15 @@ impl From<Status> for Error {
.and_then(|v| String::from_utf8(v.as_bytes().to_vec()).ok())
}
let code = get_metadata_value(&e, GREPTIME_DB_HEADER_ERROR_CODE)
.and_then(|s| {
if let Ok(code) = s.parse::<u32>() {
StatusCode::from_u32(code)
} else {
None
}
})
.unwrap_or(StatusCode::Internal);
let code = get_metadata_value(&e, GREPTIME_DB_HEADER_ERROR_CODE).and_then(|s| {
if let Ok(code) = s.parse::<u32>() {
StatusCode::from_u32(code)
} else {
None
}
});
let tonic_code = e.code();
let code = code.unwrap_or_else(|| convert_tonic_code_to_status_code(tonic_code));
let msg = get_metadata_value(&e, GREPTIME_DB_HEADER_ERROR_MSG)
.unwrap_or_else(|| e.message().to_string());
@@ -184,7 +186,8 @@ impl From<Status> for Error {
Self::MetaServer {
code,
msg,
tonic_code: e.code(),
tonic_code,
location: location!(),
}
}
}

View File

@@ -772,9 +772,14 @@ impl IntoResponse for Error {
}
}
/// Converts [StatusCode] to [HttpStatusCode].
pub fn status_code_to_http_status(status_code: &StatusCode) -> HttpStatusCode {
match status_code {
StatusCode::Success | StatusCode::Cancelled => HttpStatusCode::OK,
StatusCode::Success => HttpStatusCode::OK,
// When a request is cancelled by the client (e.g., by a client side timeout),
// we should return a gateway timeout status code to the external client.
StatusCode::Cancelled | StatusCode::DeadlineExceeded => HttpStatusCode::GATEWAY_TIMEOUT,
StatusCode::Unsupported
| StatusCode::InvalidArguments

View File

@@ -334,7 +334,7 @@ fn mysql_error_kind(status_code: &StatusCode) -> ErrorKind {
StatusCode::Success => ErrorKind::ER_YES,
StatusCode::Unknown | StatusCode::External => ErrorKind::ER_UNKNOWN_ERROR,
StatusCode::Unsupported => ErrorKind::ER_NOT_SUPPORTED_YET,
StatusCode::Cancelled => ErrorKind::ER_QUERY_INTERRUPTED,
StatusCode::Cancelled | StatusCode::DeadlineExceeded => ErrorKind::ER_QUERY_INTERRUPTED,
StatusCode::RuntimeResourcesExhausted => ErrorKind::ER_OUT_OF_RESOURCES,
StatusCode::InvalidSyntax => ErrorKind::ER_SYNTAX_ERROR,
StatusCode::RegionAlreadyExists | StatusCode::TableAlreadyExists => {

View File

@@ -374,6 +374,7 @@ impl From<StatusCode> for PgErrorCode {
StatusCode::Unsupported => PgErrorCode::Ec0A000,
StatusCode::InvalidArguments => PgErrorCode::Ec22023,
StatusCode::Cancelled => PgErrorCode::Ec57000,
StatusCode::DeadlineExceeded => PgErrorCode::Ec57000,
StatusCode::External => PgErrorCode::Ec58000,
StatusCode::Unknown