refactor(pgwire): refactor error handle #3841
Conversation
ZENOTME
force-pushed
the
zj/pgwire_error_handle_refactor
branch
2 times, most recently
from
93b4a97 to
efec891
Compare
ZENOTME
force-pushed
the
zj/pgwire_error_handle_refactor
branch
from
efec891 to
2f013a2
Compare
Codecov Report
@@ Coverage Diff @@
## main #3841 +/- ##
=======================================
Coverage 74.00% 74.00%
=======================================
Files 818 818
Lines 115304 115392 +88
=======================================
+ Hits 85331 85400 +69
- Misses 29973 29992 +19
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
BugenZhao
changed the title
| self.write_message_no_flush(&BeMessage::AuthenticationOk) | ||
| .map_err(PsqlError::PasswordError)?; |
There was a problem hiding this comment.
For write_message or write_message_no_flush, maybe we should not .map_err distinguish different type of error and all treat them like PsqlError::IoError?
There was a problem hiding this comment.
write_message_no_flush in process_password and process_startup stiil need to map as PasswordError and StartupError. Because in process_password/process_startup, if write_message_no_flush return a error, you need to close a connection rather than just report it. So we can't treat it as a PsqlError::IoError, PsqlError::IoError will just report but not close connection.
There was a problem hiding this comment.
Actually I think every Io::Error should close connection. Let me take a look
There was a problem hiding this comment.
In past version, I think most of Io::Error don't close connection.
ZENOTME
force-pushed
the
zj/pgwire_error_handle_refactor
branch
2 times, most recently
from
e5885ce to
6ea7d43
Compare
| @@ -131,7 +128,11 @@ where | |||
| Err(e) => { | |||
| tracing::error!("Error: {}", e); | |||
| match e { | |||
| PsqlError::CancelError(_) | PsqlError::SslError(_) | PsqlError::IoError(_) => {} | |||
| PsqlError::SslError(io_err) | PsqlError::IoError(io_err) => { | |||
There was a problem hiding this comment.
Better use '@' here.
https://stackoverflow.com/a/49906536
There was a problem hiding this comment.
How to use '@' here..
src/utils/pgwire/src/pg_protocol.rs
Outdated
| } | ||
|
|
||
| async fn do_process( | ||
| async fn do_process_error( |
There was a problem hiding this comment.
Suggest change the name here..
do_process and do_process_inner?
ZENOTME
force-pushed
the
zj/pgwire_error_handle_refactor
branch
from
6ea7d43 to
22df2cc
Compare
* add error type * modify pg_server.rs * modified error type * modify error * refactor do_process to more clear * fix clippy * refactor error handle * modify error hanlde Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.
What's changed and what's your intention?
refactor error handle in pgwire
Please explain IN DETAIL what the changes are in this PR and why they are needed:
Checklist
./risedev check(or alias,./risedev c)Documentation
If your pull request contains user-facing changes, please specify the types of the changes, and create a release note. Otherwise, please feel free to remove this section.
Types of user-facing changes
Please keep the types that apply to your changes, and remove those that do not apply.
Release note
#3829
Please create a release note for your changes. In the release note, focus on the impact on users, and mention the environment or conditions where the impact may occur.
Refer to a related PR or issue link (optional)