-
Notifications
You must be signed in to change notification settings - Fork 524
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
store-gateway series: break up fetching of series into functions #3540
store-gateway series: break up fetching of series into functions #3540
Conversation
s.metrics.seriesGetAllDuration.Observe(stats.getAllDuration.Seconds()) | ||
s.metrics.seriesBlocksQueried.Observe(float64(stats.blocksQueried)) |
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 was also moved to recordSeriesCallResult
67eaa19
to
ec5e732
Compare
pkg/storegateway/bucket.go
Outdated
@@ -840,10 +840,8 @@ func (s *BucketStore) Series(req *storepb.SeriesRequest, srv storepb.Store_Serie | |||
|
|||
var ( | |||
ctx = srv.Context() | |||
stats = &queryStats{} | |||
stats *queryStats |
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.
is this declaration necessary? It just gets re-declared further down
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 should have been fixed with 0f75572
Series() acquires a per-tenant lock when starting to read from a block. The reason is to prevent a race between addBlock, removeBlock, and reading from these blocks (for example a block is unloaded after Series () has already selected it for querying; when accessing this block's index header we will be using an mmaped file which was deleted from disk). This PR moves this locking outside of Series() so we can use further simplify the code in PR 3540 (see [comment](#3540 (comment))) Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Series() acquires a per-tenant lock when starting to read from a block. The reason is to prevent a race between addBlock, removeBlock, and reading from these blocks (for example a block is unloaded after Series () has already selected it for querying; when accessing this block's index header we will be using an mmaped file which was deleted from disk). This PR moves this locking outside of Series() so we can use further simplify the code in PR 3540 (see [comment](#3540 (comment))) Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
This PR only moves code out of the Series method. It moved two functions - one that queries all blocks and one that updates metrics of the store-gateway from the stats of each Series method call Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
0f75572
to
1f015ec
Compare
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.
Good job, overall LGTM! I left few minor things I would like to see fixed before we merge. Thanks!
Co-authored-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
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, thanks!
Series() acquires a per-tenant lock when starting to read from a block. The reason is to prevent a race between addBlock, removeBlock, and reading from these blocks (for example a block is unloaded after Series () has already selected it for querying; when accessing this block's index header we will be using an mmaped file which was deleted from disk). This PR moves this locking outside of Series() so we can use further simplify the code in PR 3540 (see [comment](grafana#3540 (comment))) Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com> Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
…fana#3540) This PR only moves code out of the Series method. It moves two functions - one that queries all blocks and one that updates metrics of the store-gateway from the stats of each Series method call Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
This PR only moves code out of the Series method. It moved two functions -
one that queries all blocks and one that updates metrics of the
store-gateway from the stats of each Series method call