Fixed max global series per user/metric limit when shuffle sharding is enabled #3369
Conversation
|
|
pracucci
force-pushed
the
fix-global-limits-when-shuffle-sharding-is-enabled
branch
from
de8185a to
604ba5b
Compare
|
|
||
| - `max_series_per_query` / `-ingester.max-series-per-query` | ||
| - `max_samples_per_query` / `-ingester.max-samples-per-query` | ||
|
|
||
| Limits on the number of timeseries and samples returns by a single ingester during a query. | ||
|
|
||
| - `max_metadata_per_user` / `-ingester.max-metadata-per-user` |
There was a problem hiding this comment.
Metadata limits were not documented, do I've added it.
| } | ||
|
|
||
| func (f *flushTransferer) Flush() {} | ||
| func (f *flushTransferer) TransferOut(ctx context.Context) error { |
There was a problem hiding this comment.
Unless I'm missing anything, the logic of this flushTransferer was not used, so I've removed it in favour of the tiny nopFlushTransferer
| if numIngesters > 0 { | ||
| return int((float64(globalLimit) / float64(numIngesters)) * float64(l.replicationFactor)) | ||
| if numIngesters == 0 { | ||
| return 0 |
There was a problem hiding this comment.
So now local limit is 0 if num healthy ingesters is zero, right? I think that's probably okay, but the comment above is confusing now, since that's not what "ignore the global limit" makes me think.
There was a problem hiding this comment.
Let me also outline the logic hasn't changed. If the ring client doesn't see any ingester (0), the global rate limit was temporarily disabled even before.
| @@ -17,65 +18,124 @@ func TestSeriesLimit_maxSeriesPerMetric(t *testing.T) { | |||
| maxLocalSeriesPerMetric int | |||
| maxGlobalSeriesPerMetric int | |||
| ringReplicationFactor int | |||
| ringZoneAwarenessEnabled bool | |||
There was a problem hiding this comment.
Would it be possible to refactor limiter code to avoid the same repeated tests? (In a follow-up PR, if at all).
There was a problem hiding this comment.
I think could be possible. Added to my TODO list for a separate PR.
…s enabled Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
pracucci
force-pushed
the
fix-global-limits-when-shuffle-sharding-is-enabled
branch
from
43c3a84 to
5df9485
Compare
pracucci
deleted the
fix-global-limits-when-shuffle-sharding-is-enabled
branch
What this PR does:
The max global series per user/metric limit doesn't work correctly when shuffle sharding and shard-by-all-labels are both enabled because it considers series to be distributed across all ingesters, while actually series are only distributed across a subset of ingesters (the shard size).
Which issue(s) this PR fixes:
Fixes #3367
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]