Frontend middlewares refactoring #1734
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cyriltovena
force-pushed
the
queryrange-abstract
branch
3 times, most recently
from
a3be91b to
4e15bc9
Compare
owen-d
approved these changes
Oct 18, 2019
gouthamve
reviewed
Oct 22, 2019
cyriltovena
force-pushed
the
queryrange-abstract
branch
2 times, most recently
from
6d171ad to
ebc2e36
Compare
|
Can we retrigger the build ? it had a glitch, circleci .... |
gouthamve
approved these changes
Oct 24, 2019
There was a problem hiding this comment.
LGTM!
One minor question I have is around just not using reserved and letting the Unmarshaling to error on the older cache entries. We can make it clear in the CHANGELOG that folks should nuke the cache on update.
Also, @cyriltovena could you update the changelog to say we moved to a new format and that the older entries will be automatically expired?
cyriltovena
force-pushed
the
queryrange-abstract
branch
from
f7cd6d6 to
21c6cc2
Compare
cyriltovena
force-pushed
the
queryrange-abstract
branch
4 times, most recently
from
6c5f156 to
b8d718e
Compare
jtlisi
suggested changes
Oct 28, 2019
cyriltovena
force-pushed
the
queryrange-abstract
branch
from
b8d718e to
645f1ae
Compare
The behaviour of both packages hasn't changed. Middlewares now uses `Request` and `Response` interfaces . This allows to create a roundtripper that will encode/decode HTTP requests/responses (using a `Codec`) to wrap the middleware chain. To support this new Response interface the cache protobuf message has been updated with an [`Any`](https://developers.google.com/protocol-buffers/docs/proto3#any) field. I have also updated the code so that if the field doesn't exist, it will be trigger a cache miss. The queryrange configuration has moved but flag definition remains the same, this allows seamless update if you use flag configuration. However configuration by file must be updated to new field `query_range` at root. Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
cyriltovena
force-pushed
the
queryrange-abstract
branch
from
645f1ae to
ec69629
Compare
cyriltovena
commented
Oct 29, 2019
| if maxQueryLen != 0 && queryLen > maxQueryLen { | ||
| return nil, httpgrpc.Errorf(http.StatusBadRequest, validation.ErrQueryTooLong, queryLen, maxQueryLen) | ||
| } | ||
| LogToSpan(r.Context(), request) |
There was a problem hiding this comment.
FYI @jtlisi @gouthamve I had to rebase to master and re-introduce this.
see https://github.com/cortexproject/cortex/pull/1734/files#diff-19934b07a15ed5212e1637c543973bd1R79
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This makes the
frontendandqueryrangepackage easier to use by external projects. The behaviour of both packages hasn't changed.Middlewares now uses
RequestandResponseinterfaces . This allows to create a roundtripper that will encode/decode HTTP requests/responses (using aCodec) to wrap the middleware chain.To support this new Response interface the cache protobuf message has been updated with an
Anyfield. I have also updated the code so that if the field doesn't exist, it will be trigger a cache miss.I've added some tests for the cache migration but also tested this in our dev environment.
The queryrange configuration has moved but flag definition remains the same, this allows seamless update if you use flag configuration. However configuration by file must be updated to new field
query_rangeat root./cc @owen-d @gouthamve can you please review this.
Next steps: