-
Notifications
You must be signed in to change notification settings - Fork 60
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
encapsulate httpgrpc to http request unwrapping as public fn #434
encapsulate httpgrpc to http request unwrapping as public fn #434
Conversation
// HTTPRequest wraps an ordinary HTTPRequest with a gRPC one | ||
func HTTPRequest(r *http.Request) (*httpgrpc.HTTPRequest, error) { | ||
// WrapHTTPRequest wraps an ordinary http.Request up into an httpgrpc.HTTPRequest | ||
func WrapHTTPRequest(r *http.Request) (*httpgrpc.HTTPRequest, error) { |
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.
there is one usage of this in Mimir/GEM that would need updated with the new name, but if we're super adverse to changing that, it could remain as it was.
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's not "wrapping" (new message doesn't keep original request), but full conversion, so the new naming is wrong misleading.
toHeader(r.Headers, req.Header) | ||
req = req.WithContext(ctx) | ||
req.RequestURI = r.Url | ||
req.ContentLength = int64(len(r.Body)) |
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.
We don't need to repeat these steps in Handle
above, do we?
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.
yep, just fixed
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!
What this PR does:
this moves the httpgrpc request -> http request conversion to a public func, and renames the existing opposite conversion func to match.
For the multi-dimensional scheduler queue work, we need to route queries based on whether they will touch ingesters or store gateways.
We want to re-use the existing code for this for obvious reasons - Mimir
querymiddleware
split and cache functionality already handles this by converting anhttp.Request
to aquerymiddleware.Request
type representing either an instant or range request.In the scheduler queueing code, we are starting with the httpgrpc request rather than the unwrapped
http.Request
so we need to unwrap it ourselves using this newly-exposed func before we can re-use the other querymiddleware code mentioned above.Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]