Initial query limit overriding #3090
Conversation
rallen090
requested review from
arnikola,
ryanhall07,
robskillington and
wesleyk
Codecov Report
@@ Coverage Diff @@
## master #3090 +/- ##
=========================================
+ Coverage 71.9% 72.2% +0.3%
=========================================
Files 1083 1084 +1
Lines 100592 100162 -430
=========================================
+ Hits 72371 72399 +28
+ Misses 23200 22722 -478
- Partials 5021 5041 +20
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
| @@ -595,7 +595,6 @@ func (r *blockRetriever) streamRequest( | |||
| shard uint32, | |||
| id ident.ID, | |||
| startTime time.Time, | |||
| nsCtx namespace.Context, | |||
There was a problem hiding this comment.
Linter was complaining
| kvStore kv.Store, | ||
| update *KeyValueUpdate, | ||
| ) (*KeyValueUpdateResult, error) { | ||
| old, err := kvStore.Get(update.Key) |
There was a problem hiding this comment.
Allowing arbitrary kv updates here is a little scary; seems the footgunning potential may be quite high, although forcing the Commit flag may be enough for safety
There was a problem hiding this comment.
Well so they have to conform to the type of input (e.g. the key for querylimits must be as kvpb.QueryLimits{}). And yeah I think the Commit allows you to safely confirm the diff which we return in the result before applying.
There was a problem hiding this comment.
Could we use CheckAndSet instead to require the client to know what version is being overridden?
| @@ -53,6 +53,16 @@ per second safely with your deployment and you want to use the default lookback | |||
| of `15s` then you would multiply 10,000 by 15 to get 150,000 as a max value with | |||
| a 15s lookback. | |||
|
|
|||
| The third limit `maxRecentlyQueriedSeriesDiskRead` caps the bytes associated with | |||
There was a problem hiding this comment.
Should we make each of the limits a subsection so it's clear what each section pertains to?
| `maxRecentlyQueriedSeriesBlocks`, which also limits the memory cost of specific series | ||
| matched in-memory, because of an inefficiency in how allocations would occur even for series | ||
| known to not be present on disk for a given shard. This inefficiency has been resolved | ||
| https://github.com/m3db/m3/pull/3103 and therefore this limit should be tracking memory cost |
There was a problem hiding this comment.
can we make this a link like "resolved with this PR and..."
| kvStore kv.Store, | ||
| update *KeyValueUpdate, | ||
| ) (*KeyValueUpdateResult, error) { | ||
| old, err := kvStore.Get(update.Key) |
There was a problem hiding this comment.
Could we use CheckAndSet instead to require the client to know what version is being overridden?
| }, | ||
| }, | ||
| commit: true, | ||
| expectedJSON: `maxRecentlyQueriedSeriesBlocks:<limit:1 lookbackSeconds:15 forceExceeded:true > `, |
There was a problem hiding this comment.
out of curiosity what's up with the formatting at the end (true > )
There was a problem hiding this comment.
Oh so the true is the val of forceExceeded and the > is the close bracket for the json.RawMessage
There was a problem hiding this comment.
(was more curious why the spacing ends up like that instead of the beginning, which doesn't have any spaces, not really important)
* master: [DOCS] Update to cluster docs (#3084) [dbnode][coordinator] Ensure docs limit is propagated for search and aggregate RPCs (#3108) [query] Take bounds into account for list endpoints (#3110) Add warning to changing blocksize (#3096) Add support for dynamic query limit overriding (#3090) [tests] test setups exported to allow us to use it from other packages (#3042) [query] Implemented Graphite's pow function (#3048) [dbnode] Direct conversion of encoded tags to doc.Metadata (#3087) [tests] Skip flaky TestWatchNoLeader (#3106) [dbnode] Faster search of tag bytes in convert.FromSeriesIDAndTags (#3075) Replace bytes.Compare() == 0 with bytes.Equal() (#3101) Capture seekerMgr instead Rlock (#3104) [m3db] Check bloom filter before stream request allocation (#3103)
What this PR does / why we need it:
To support dynamically overriding query limit values.
E.g. when running
this then results in forced query abandoning
Special notes for your reviewer:
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: