-
Notifications
You must be signed in to change notification settings - Fork 315
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add a new status code for "external" errors #4775
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4775 +/- ##
==========================================
- Coverage 84.47% 84.46% -0.02%
==========================================
Files 1118 1118
Lines 203093 203127 +34
==========================================
+ Hits 171571 171572 +1
- Misses 31522 31555 +33 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can understand the rational for this. But from the end user perspective, this type of error is still Internal ones for a database.
Imagine in our cloud service, the authentication backend is built and maintained by ourselves, this is not essential to expose this type of errors to end user.
Of course we need to log this type of errors for database admin to track
@sunng87 I think the demand for the status code for "external" errors do exist, especially when greptimedb will have connected to external systems. Two other databases both have this kind of error codes, postgresql and clickhouse. The status codes are involved in errors, not the other way around. When we implementing features, we can freely choose to return what kind of error (and the status code) to the user. If, like in our cloud service, we need to make the external system's error internal to greptimedb, we can simply change the status code of it. |
643c867
to
1ae79ec
Compare
@shuiyisong PTAL |
@MichaelScofield Makes sense. Thank you for explanation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost LGTM
1ae79ec
to
b0e5ff1
Compare
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
This "external" status code can represent errors originated from system that is outside of greptimedb, useful for authentication.
Checklist