leekeiabstraction commented on code in PR #302:
URL: https://github.com/apache/fluss-rust/pull/302#discussion_r2797579392
##########
bindings/cpp/src/lib.rs:
##########
@@ -451,16 +451,25 @@ fn err_result(code: i32, msg: String) -> ffi::FfiResult {
}
}
+/// Convert a core Error to FfiResult, extracting the API error code when
available.
+fn err_from_core_error(e: &fcore::error::Error) -> ffi::FfiResult {
Review Comment:
Should we also have a constant file like in Rust's `FlussError`? So that
users do not have to maintain their own constants
##########
bindings/python/src/error.rs:
##########
@@ -24,22 +24,41 @@ use pyo3::prelude::*;
pub struct FlussError {
#[pyo3(get)]
pub message: String,
+ #[pyo3(get)]
+ pub error_code: i32,
}
#[pymethods]
impl FlussError {
#[new]
- fn new(message: String) -> Self {
- Self { message }
+ #[pyo3(signature = (message, error_code=0))]
+ fn new(message: String, error_code: i32) -> Self {
+ Self {
+ message,
+ error_code,
+ }
}
fn __str__(&self) -> String {
- format!("FlussError: {}", self.message)
+ if self.error_code != 0 {
+ format!("FlussError(code={}): {}", self.error_code, self.message)
+ } else {
+ format!("FlussError: {}", self.message)
+ }
}
}
impl FlussError {
pub fn new_err(message: impl ToString) -> PyErr {
- PyErr::new::<FlussError, _>(message.to_string())
+ PyErr::new::<FlussError, _>((message.to_string(), 0i32))
+ }
+
+ /// Create a PyErr from a core Error, extracting the API error code if
available.
+ pub fn from_core_error(error: &fluss::error::Error) -> PyErr {
+ if let fluss::error::Error::FlussAPIError { api_error } = error {
+ PyErr::new::<FlussError, _>((api_error.message.clone(),
api_error.code))
+ } else {
+ PyErr::new::<FlussError, _>((error.to_string(), 0i32))
Review Comment:
Magic number?
##########
bindings/cpp/src/lib.rs:
##########
@@ -451,16 +451,25 @@ fn err_result(code: i32, msg: String) -> ffi::FfiResult {
}
}
+/// Convert a core Error to FfiResult, extracting the API error code when
available.
+fn err_from_core_error(e: &fcore::error::Error) -> ffi::FfiResult {
+ if let fcore::error::Error::FlussAPIError { api_error } = e {
+ err_result(api_error.code, api_error.message.clone())
+ } else {
+ err_result(-1, e.to_string())
Review Comment:
Wouldn't this cause user to be unable to distinguish non FlussAPIError and
FlussAPIError?
UnknownServerError is mapped to `-1` on Rust and Java side
##########
bindings/cpp/src/lib.rs:
##########
Review Comment:
Should we consider updating this as well? Actually, I do not understand the
error code used for some of these calls. Most of them are called with hardcoded
1 which maps to API Error's `NetworkException`, I suspect that is incorrect. We
should have error constants for client side exception as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
