store/bucket: make getFor() work with interleaved resolutions #1146
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -675,7 +675,7 @@ func populateChunk(out *storepb.AggrChunk, in chunkenc.Chunk, aggrs []storepb.Ag | |
| // labels and resolution. This is important because we allow mixed resolution results, so it is quite crucial | ||
| // to be aware what exactly resolution we see on query. | ||
| // TODO(bplotka): Consider adding resolution label to all results to propagate that info to UI and Query API. | ||
| func debugFoundBlockSetOverview(logger log.Logger, mint, maxt, maxResolutionMillis int64, lset labels.Labels, bs []*bucketBlock) { | ||
| if len(bs) == 0 { | ||
| level.Debug(logger).Log("msg", "No block found", "mint", mint, "maxt", maxt, "lset", lset.String()) | ||
| return | ||
|
|
@@ -703,7 +703,7 @@ func debugFoundBlockSetOverview(logger log.Logger, mint, maxt int64, lset labels | |
|
|
||
| parts = append(parts, fmt.Sprintf("Range: %d-%d Resolution: %d", currMin, currMax, currRes)) | ||
|
|
||
| level.Debug(logger).Log("msg", "Blocks source resolutions", "blocks", len(bs), "Maximum Resolution", maxResolutionMillis, "mint", mint, "maxt", maxt, "lset", lset.String(), "spans", strings.Join(parts, "\n")) | ||
| } | ||
|
|
||
| // Series implements the storepb.StoreServer interface. | ||
|
|
@@ -738,7 +738,7 @@ func (s *BucketStore) Series(req *storepb.SeriesRequest, srv storepb.Store_Serie | |
| blocks := bs.getFor(req.MinTime, req.MaxTime, req.MaxResolutionWindow) | ||
|
|
||
| if s.debugLogging { | ||
| debugFoundBlockSetOverview(s.logger, req.MinTime, req.MaxTime, req.MaxResolutionWindow, bs.labels, blocks) | ||
| } | ||
|
|
||
| for _, b := range blocks { | ||
|
|
@@ -934,7 +934,7 @@ func (s *BucketStore) LabelValues(ctx context.Context, req *storepb.LabelValuesR | |
| type bucketBlockSet struct { | ||
| labels labels.Labels | ||
| mtx sync.RWMutex | ||
| resolutions []int64 // available resolution, high to low (in milliseconds) | ||
| blocks [][]*bucketBlock // ordered buckets for the existing resolutions | ||
| } | ||
|
|
||
|
|
@@ -996,8 +996,8 @@ func int64index(s []int64, x int64) int { | |
| } | ||
|
|
||
| // getFor returns a time-ordered list of blocks that cover date between mint and maxt. | ||
| // Blocks with the biggest resolution possible but not bigger than the given max resolution are returned. | ||
| func (s *bucketBlockSet) getFor(mint, maxt, maxResolutionMillis int64) (bs []*bucketBlock) { | ||
| if mint == maxt { | ||
| return nil | ||
| } | ||
|
|
@@ -1007,7 +1007,7 @@ func (s *bucketBlockSet) getFor(mint, maxt, minResolution int64) (bs []*bucketBl | |
|
|
||
| // Find first matching resolution. | ||
| i := 0 | ||
| for ; i < len(s.resolutions) && s.resolutions[i] > maxResolutionMillis; i++ { | ||
| } | ||
|
|
||
| // Base case, we fill the given interval with the closest resolution. | ||
|
|
@@ -1022,6 +1022,8 @@ func (s *bucketBlockSet) getFor(mint, maxt, minResolution int64) (bs []*bucketBl | |
| } | ||
| // Our current resolution might not cover all data, recursively fill the gaps at the start | ||
| // and end of [mint, maxt] with higher resolution blocks. | ||
| // | ||
| // Plus, fill the possible gaps between the current blocks with higher resolution blocks. | ||
| i++ | ||
| // No higher resolution left, we are done. | ||
| if i >= len(s.resolutions) { | ||
|
|
@@ -1030,6 +1032,19 @@ func (s *bucketBlockSet) getFor(mint, maxt, minResolution int64) (bs []*bucketBl | |
| if len(bs) == 0 { | ||
| return s.getFor(mint, maxt, s.resolutions[i]) | ||
| } | ||
|
|
||
| until := len(bs) - 1 | ||
| for j := 0; j < until; j++ { | ||
|
There was a problem hiding this comment. It's tricky, need to dive more but generally makes sense. Explaining what we are seeing now. Thanks for tests catching this as well! All of this assumes that blocks within different resolutions are aligned ideally right? Can we comment this somehow? Also be aware of this: #1104 Hope you algorthim would be nicely extendable I guess. There was a problem hiding this comment. They always should be aligned ideally or on the same timestamps because in other cases we would get a problem known as "overlapping blocks". This change should be easy to integrate into that PR. There was a problem hiding this comment. OK, It makes total sense BUT to me Can we remove this left and right and treat them properly in this loop as There was a problem hiding this comment. The idea sounds good and I played around with it a bit but I doubt that this function can be made more succinct because we have three distinct cases of the There was a problem hiding this comment. Hm, is it really too complex? I will propose something to your PR There was a problem hiding this comment. Proposed: GiedriusS#1 IMO it's way simpler, what do you think? @GiedriusS // getFor returns a time-ordered list of blocks that cover date between mint and maxt.
// Blocks with the biggest resolution possible but not bigger than the given max resolution are returned.
func (s *bucketBlockSet) getFor(mint, maxt, maxResolutionMillis int64) (bs []*bucketBlock) {
if mint == maxt {
return nil
}
s.mtx.RLock()
defer s.mtx.RUnlock()
// Find first matching resolution.
i := 0
for ; i < len(s.resolutions) && s.resolutions[i] > maxResolutionMillis; i++ {
}
// Fill the given interval with the blocks for the current resolution.
// Our current resolution might not cover all data, so recursively fill the gaps with higher resolution blocks if there is any.
start := mint
for _, b := range s.blocks[i] {
if b.meta.MaxTime <= mint {
continue
}
if b.meta.MinTime >= maxt {
break
}
if i+1 < len(s.resolutions) {
bs = append(bs, s.getFor(start, b.meta.MinTime, s.resolutions[i+1])...)
}
bs = append(bs, b)
start = b.meta.MaxTime
}
if i+1 < len(s.resolutions) {
bs = append(bs, s.getFor(start, maxt, s.resolutions[i+1])...)
}
return bs
} |
||
| if bs[j+1].meta.MinTime-bs[j].meta.MaxTime > 0 { | ||
| between := s.getFor(bs[j].meta.MaxTime, bs[j+1].meta.MinTime, s.resolutions[i]) | ||
| bs = append(bs[:j+1], append(between, bs[j+1:]...)...) | ||
|
|
||
| // Push the iterators further. | ||
| j += len(between) | ||
| until += len(between) | ||
| } | ||
| } | ||
|
|
||
| left := s.getFor(mint, bs[0].meta.MinTime, s.resolutions[i]) | ||
| right := s.getFor(bs[len(bs)-1].meta.MaxTime, maxt, s.resolutions[i]) | ||
|
|
||
|
|
||
Oops, something went wrong.
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.
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.
👍