-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[ENH] handle panics in query service #2375
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
service Debug { | ||
rpc GetInfo(google.protobuf.Empty) returns (GetInfoResponse) {} | ||
rpc TriggerPanic(google.protobuf.Empty) returns (google.protobuf.Empty) {} | ||
} |
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.
suggestions welcome for a cleaner way to do this, I just want a way to trigger a panic within a service from a test
} | ||
|
||
service Debug { | ||
rpc GetInfo(google.protobuf.Empty) returns (GetInfoResponse) {} |
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.
It this just so when testing manually you can check its up?
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 need a handler that returns a successful response without panicking. Alternatively I could add a shoud_panic
option to the request for TriggerPanic
, or call a normal rpc route like QueryVectors
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.
Got it, thats fine makes sense
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.
Makes sense, just a nit on comments. Are we adding the panic hook in another PR?
yep 👍 |
This PR adds a test to confirm Tonic handles panics in the simplest cases for the query node.
Panics within components are untested (I suspect they wouldn't be caught by this).