-
Notifications
You must be signed in to change notification settings - Fork 403
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
Enable DefaultHTTPErrorHandler and Upgrade grpc-gateway to v2 #369
Conversation
apiserver/cmd/main.go
Outdated
@@ -4,14 +4,16 @@ import ( | |||
"context" | |||
"flag" | |||
|
|||
"google.golang.org/protobuf/encoding/protojson" | |||
|
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.
should run gofmt here.
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.
Let me rerun it
@@ -4,6 +4,8 @@ import ( | |||
"context" | |||
"fmt" | |||
|
|||
"k8s.io/apimachinery/pkg/api/errors" | |||
|
|||
"github.com/ray-project/kuberay/apiserver/pkg/model" | |||
|
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.
nit: need to remove space here.
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.
this is some fmt issue that goimports
and gofmt
can not detect. that's why CI pass. I manually update the format and it also works
1. Enable `runtime.DefaultHTTPErrorHandler` to convert gRPC status code to HTTP code. 2. Since we use custom error struct, we need to implement `GRPCStatus` to make sure grpc-go can recognize the error code in our struct, Otherwise, it will always return codes.Unknown(2), that's the reason we always see code 2 and HTTP 500 in the past.
@scarlet25151 comments addressed. Please take another look |
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.
LGTM.
…oject#369) * Upgrade grpc-gateway to v2 * Customize error and enable DefaultHTTPErrorHandler 1. Enable `runtime.DefaultHTTPErrorHandler` to convert gRPC status code to HTTP code. 2. Since we use custom error struct, we need to implement `GRPCStatus` to make sure grpc-go can recognize the error code in our struct, Otherwise, it will always return codes.Unknown(2), that's the reason we always see code 2 and HTTP 500 in the past.
Why are these changes needed?
Related issue number
#364
Checks