Skip to content
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.

Sign up for GitHub

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

Jump to bottom

store/bucket: make getFor() work with interleaved resolutions #1146

Merged
merged 25 commits into from
May 27, 2019
Merged

store/bucket: make getFor() work with interleaved resolutions #1146

Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
9c94918
store/bucket_test: add interleaved resolutions test for getFor()
May 15, 2019
cd3363c
store/bucket: include blocks in the middle as well
May 15, 2019
00b2c7c
store/bucket: add test cases with duplicated time ranges
May 16, 2019
8f22504
query/querier: send proper maxSourceResolution
May 16, 2019
e2c46bf
README: add entry
May 16, 2019
342aede
query/querier_test: add queryableCreator test
May 16, 2019
4e73b2f
store/bucket: do the iteration without sorting
May 16, 2019
fae1929
store/bucket: bsi->j in loop
May 16, 2019
bf12553
Merge remote-tracking branch 'origin/master' into getFor
May 17, 2019
de4b79a
store/bucket: fix according to review comments
May 17, 2019
c3abb09
query/querier_test: fix test
May 17, 2019
f22cffa
*: clarify everywhere that max source resolution is in millis
May 17, 2019
5e9d0e9
*: maxSourceResolutionMillis -> maxResolutionMillis
May 17, 2019
420ebe0
CHANGELOG: update
May 17, 2019
e8b3189
query/querier_test: fix
May 17, 2019
bacfd06
store/bucket: add gets all data in range property test
May 17, 2019
93a764c
store/bucket_test: add production property test
May 17, 2019
f4b0a66
store/bucket_test: fix
May 17, 2019
dac133e
store/bucket_test: add always gets property
May 17, 2019
dbe3727
query/querier_test: do not shrink
May 17, 2019
6dac954
store/bucket: revert change
May 17, 2019
55aa32d
store/bucket_test: remove more confusion
May 18, 2019
3fe4217
store/bucket: clean up tests
May 22, 2019
3bbc6cb
Simplified goFor implementation.
bwplotka May 27, 2019
3602678
Merge pull request #1 from improbable-eng/getFor
May 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
store/bucket: bsi->j in loop 
Makes it clearer that it's just a temporary variable for the loop.
  • Loading branch information
Giedrius Statkevičius committed May 16, 2019
commit fae1929018f3759041139313b2405ae3b0301bbc
10 changes: 5 additions & 5 deletions pkg/store/bucket.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1034,13 +1034,13 @@ func (s *bucketBlockSet) getFor(mint, maxt, minResolution int64) (bs []*bucketBl
}

until := len(bs) - 1
for bsi := 0; bsi < until; bsi++ {
if bs[bsi+1].meta.MinTime-bs[bsi].meta.MaxTime > 0 {
between := s.getFor(bs[bsi].meta.MaxTime, bs[bsi+1].meta.MinTime, s.resolutions[i])
bs = append(bs[:bsi+1], append(between, bs[bsi+1:]...)...)
for j := 0; j < until; j++ {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
cc @mjd95

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@bwplotka bwplotka May 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, It makes total sense BUT to me left := s.getFor(mint, bs[0].meta.MinTime, s.resolutions[i]) and right are just between 0 and first min and lastMax and maxt, right?

Can we remove this left and right and treat them properly in this loop as between? I think it would simplify flow and algorithm.

Copy link
Member Author

@GiedriusS GiedriusS May 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 append call: at the beginning of the array, between two elements, and at the end. Please correct me if I'm wrong.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, is it really too complex? I will propose something to your PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
bsi += len(between)
j += len(between)
until += len(between)
}
}
Expand Down