Skip to content

Commit 746b4e2

Browse files
WenyXuWenyXu
authored
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
1 parent 6c66ec3 commit 746b4e2

File tree

9 files changed

+53
-32
lines changed

9 files changed

+53
-32
lines changed

Diff for: ‎src/client/src/error.rs

+10-13
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
use std::any::Any;
1616

1717
use common_error::ext::{BoxedError, ErrorExt};
18-
use common_error::status_code::StatusCode;
18+
use common_error::status_code::{convert_tonic_code_to_status_code, StatusCode};
1919
use common_error::{GREPTIME_DB_HEADER_ERROR_CODE, GREPTIME_DB_HEADER_ERROR_MSG};
2020
use common_macro::stack_trace_debug;
2121
use snafu::{location, Location, Snafu};
@@ -152,15 +152,15 @@ impl From<Status> for Error {
152152
.and_then(|v| String::from_utf8(v.as_bytes().to_vec()).ok())
153153
}
154154

155-
let code = get_metadata_value(&e, GREPTIME_DB_HEADER_ERROR_CODE)
156-
.and_then(|s| {
157-
if let Ok(code) = s.parse::<u32>() {
158-
StatusCode::from_u32(code)
159-
} else {
160-
None
161-
}
162-
})
163-
.unwrap_or(StatusCode::Unknown);
155+
let code = get_metadata_value(&e, GREPTIME_DB_HEADER_ERROR_CODE).and_then(|s| {
156+
if let Ok(code) = s.parse::<u32>() {
157+
StatusCode::from_u32(code)
158+
} else {
159+
None
160+
}
161+
});
162+
let tonic_code = e.code();
163+
let code = code.unwrap_or_else(|| convert_tonic_code_to_status_code(tonic_code));
164164

165165
let msg = get_metadata_value(&e, GREPTIME_DB_HEADER_ERROR_MSG)
166166
.unwrap_or_else(|| e.message().to_string());
@@ -187,9 +187,6 @@ impl Error {
187187
} | Self::RegionServer {
188188
code: Code::Unavailable,
189189
..
190-
} | Self::RegionServer {
191-
code: Code::Unknown,
192-
..
193190
}
194191
)
195192
}

Diff for: ‎src/client/src/region.rs

+1-2
Original file line numberDiff line numberDiff line change
@@ -201,12 +201,11 @@ impl RegionRequester {
201201
.await
202202
.map_err(|e| {
203203
let code = e.code();
204-
let err: error::Error = e.into();
205204
// Uses `Error::RegionServer` instead of `Error::Server`
206205
error::Error::RegionServer {
207206
addr,
208207
code,
209-
source: BoxedError::new(err),
208+
source: BoxedError::new(error::Error::from(e)),
210209
location: location!(),
211210
}
212211
})?

Diff for: ‎src/common/error/src/status_code.rs

+15-1
Original file line numberDiff line numberDiff line change
@@ -34,12 +34,14 @@ pub enum StatusCode {
3434
Internal = 1003,
3535
/// Invalid arguments.
3636
InvalidArguments = 1004,
37-
/// The task is cancelled.
37+
/// The task is cancelled (typically caller-side).
3838
Cancelled = 1005,
3939
/// Illegal state, can be exposed to users.
4040
IllegalState = 1006,
4141
/// Caused by some error originated from external system.
4242
External = 1007,
43+
/// The request is deadline exceeded (typically server-side).
44+
DeadlineExceeded = 1008,
4345
// ====== End of common status code ================
4446

4547
// ====== Begin of SQL related status code =========
@@ -142,6 +144,7 @@ impl StatusCode {
142144
| StatusCode::Unexpected
143145
| StatusCode::InvalidArguments
144146
| StatusCode::Cancelled
147+
| StatusCode::DeadlineExceeded
145148
| StatusCode::InvalidSyntax
146149
| StatusCode::DatabaseAlreadyExists
147150
| StatusCode::PlanQuery
@@ -177,6 +180,7 @@ impl StatusCode {
177180
| StatusCode::Unexpected
178181
| StatusCode::Internal
179182
| StatusCode::Cancelled
183+
| StatusCode::DeadlineExceeded
180184
| StatusCode::IllegalState
181185
| StatusCode::EngineExecuteQuery
182186
| StatusCode::StorageUnavailable
@@ -272,6 +276,7 @@ pub fn status_to_tonic_code(status_code: StatusCode) -> Code {
272276
Code::InvalidArgument
273277
}
274278
StatusCode::Cancelled => Code::Cancelled,
279+
StatusCode::DeadlineExceeded => Code::DeadlineExceeded,
275280
StatusCode::TableAlreadyExists
276281
| StatusCode::TableColumnExists
277282
| StatusCode::RegionAlreadyExists
@@ -299,6 +304,15 @@ pub fn status_to_tonic_code(status_code: StatusCode) -> Code {
299304
}
300305
}
301306

307+
/// Converts tonic [Code] to [StatusCode].
308+
pub fn convert_tonic_code_to_status_code(code: Code) -> StatusCode {
309+
match code {
310+
Code::Cancelled => StatusCode::Cancelled,
311+
Code::DeadlineExceeded => StatusCode::DeadlineExceeded,
312+
_ => StatusCode::Internal,
313+
}
314+
}
315+
302316
#[cfg(test)]
303317
mod tests {
304318
use strum::IntoEnumIterator;

Diff for: ‎src/meta-client/src/client/cluster.rs

+2-1
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ use common_meta::rpc::store::{
3030
BatchPutResponse, DeleteRangeRequest, DeleteRangeResponse, PutRequest, PutResponse,
3131
RangeRequest, RangeResponse,
3232
};
33-
use common_telemetry::{info, warn};
33+
use common_telemetry::{error, info, warn};
3434
use snafu::{ensure, ResultExt};
3535
use tokio::sync::RwLock;
3636
use tonic::codec::CompressionEncoding;
@@ -237,6 +237,7 @@ impl Inner {
237237
times += 1;
238238
continue;
239239
} else {
240+
error!("An error occurred in gRPC, status: {status}");
240241
return Err(Error::from(status));
241242
}
242243
}

Diff for: ‎src/meta-client/src/client/procedure.rs

+2-1
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ use api::v1::meta::{
2424
};
2525
use common_grpc::channel_manager::ChannelManager;
2626
use common_telemetry::tracing_context::TracingContext;
27-
use common_telemetry::{info, warn};
27+
use common_telemetry::{error, info, warn};
2828
use snafu::{ensure, ResultExt};
2929
use tokio::sync::RwLock;
3030
use tonic::codec::CompressionEncoding;
@@ -199,6 +199,7 @@ impl Inner {
199199
times += 1;
200200
continue;
201201
} else {
202+
error!("An error occurred in gRPC, status: {status}");
202203
return Err(error::Error::from(status));
203204
}
204205
}

Diff for: ‎src/meta-client/src/error.rs

+15-12
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,10 @@
1313
// limitations under the License.
1414

1515
use common_error::ext::ErrorExt;
16-
use common_error::status_code::StatusCode;
16+
use common_error::status_code::{convert_tonic_code_to_status_code, StatusCode};
1717
use common_error::{GREPTIME_DB_HEADER_ERROR_CODE, GREPTIME_DB_HEADER_ERROR_MSG};
1818
use common_macro::stack_trace_debug;
19-
use snafu::{Location, Snafu};
19+
use snafu::{location, Location, Snafu};
2020
use tonic::Status;
2121

2222
#[derive(Snafu)]
@@ -35,6 +35,8 @@ pub enum Error {
3535
code: StatusCode,
3636
msg: String,
3737
tonic_code: tonic::Code,
38+
#[snafu(implicit)]
39+
location: Location,
3840
},
3941

4042
#[snafu(display("No leader, should ask leader first"))]
@@ -168,23 +170,24 @@ impl From<Status> for Error {
168170
.and_then(|v| String::from_utf8(v.as_bytes().to_vec()).ok())
169171
}
170172

171-
let code = get_metadata_value(&e, GREPTIME_DB_HEADER_ERROR_CODE)
172-
.and_then(|s| {
173-
if let Ok(code) = s.parse::<u32>() {
174-
StatusCode::from_u32(code)
175-
} else {
176-
None
177-
}
178-
})
179-
.unwrap_or(StatusCode::Internal);
173+
let code = get_metadata_value(&e, GREPTIME_DB_HEADER_ERROR_CODE).and_then(|s| {
174+
if let Ok(code) = s.parse::<u32>() {
175+
StatusCode::from_u32(code)
176+
} else {
177+
None
178+
}
179+
});
180+
let tonic_code = e.code();
181+
let code = code.unwrap_or_else(|| convert_tonic_code_to_status_code(tonic_code));
180182

181183
let msg = get_metadata_value(&e, GREPTIME_DB_HEADER_ERROR_MSG)
182184
.unwrap_or_else(|| e.message().to_string());
183185

184186
Self::MetaServer {
185187
code,
186188
msg,
187-
tonic_code: e.code(),
189+
tonic_code,
190+
location: location!(),
188191
}
189192
}
190193
}

Diff for: ‎src/servers/src/error.rs

+6-1
Original file line numberDiff line numberDiff line change
@@ -772,9 +772,14 @@ impl IntoResponse for Error {
772772
}
773773
}
774774

775+
/// Converts [StatusCode] to [HttpStatusCode].
775776
pub fn status_code_to_http_status(status_code: &StatusCode) -> HttpStatusCode {
776777
match status_code {
777-
StatusCode::Success | StatusCode::Cancelled => HttpStatusCode::OK,
778+
StatusCode::Success => HttpStatusCode::OK,
779+
780+
// When a request is cancelled by the client (e.g., by a client side timeout),
781+
// we should return a gateway timeout status code to the external client.
782+
StatusCode::Cancelled | StatusCode::DeadlineExceeded => HttpStatusCode::GATEWAY_TIMEOUT,
778783

779784
StatusCode::Unsupported
780785
| StatusCode::InvalidArguments

Diff for: ‎src/servers/src/mysql/writer.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -334,7 +334,7 @@ fn mysql_error_kind(status_code: &StatusCode) -> ErrorKind {
334334
StatusCode::Success => ErrorKind::ER_YES,
335335
StatusCode::Unknown | StatusCode::External => ErrorKind::ER_UNKNOWN_ERROR,
336336
StatusCode::Unsupported => ErrorKind::ER_NOT_SUPPORTED_YET,
337-
StatusCode::Cancelled => ErrorKind::ER_QUERY_INTERRUPTED,
337+
StatusCode::Cancelled | StatusCode::DeadlineExceeded => ErrorKind::ER_QUERY_INTERRUPTED,
338338
StatusCode::RuntimeResourcesExhausted => ErrorKind::ER_OUT_OF_RESOURCES,
339339
StatusCode::InvalidSyntax => ErrorKind::ER_SYNTAX_ERROR,
340340
StatusCode::RegionAlreadyExists | StatusCode::TableAlreadyExists => {

Diff for: ‎src/servers/src/postgres/types/error.rs

+1
Original file line numberDiff line numberDiff line change
@@ -374,6 +374,7 @@ impl From<StatusCode> for PgErrorCode {
374374
StatusCode::Unsupported => PgErrorCode::Ec0A000,
375375
StatusCode::InvalidArguments => PgErrorCode::Ec22023,
376376
StatusCode::Cancelled => PgErrorCode::Ec57000,
377+
StatusCode::DeadlineExceeded => PgErrorCode::Ec57000,
377378
StatusCode::External => PgErrorCode::Ec58000,
378379

379380
StatusCode::Unknown

0 commit comments

Comments
 (0)