Store: Add Time & duration based partitioning #1408

povilasv · 2019-08-13T15:33:39Z

Changes

New start of #1077

Verification

Multiple tests added.

Signed-off-by: Povilas Versockas <p.versockas@gmail.com>

bwplotka

Nice, looks good generally.

Some naming suggestions, plus one high level question:

Also even with time slicing looks like we still will download all meta.json files to disk. Is that what we want? Essentially time slicing will not include meta.json files on disk.

cmd/thanos/store.go

bwplotka · 2019-08-27T16:43:54Z

pkg/store/bucket.go

@@ -309,6 +319,17 @@ func (s *BucketStore) SyncBlocks(ctx context.Context) error {
 		if err != nil {
 			return nil
 		}
+
+		inRange, err := s.isBlockInMinMaxRange(ctx, id)


I think the problem here is that we hit the issue of partial blocks... Something that #1394 touches.

I thought we actually handle this in store gw properly (blocks without meta are excluded), seems not so not a regression.

In any case we will just warn so 👍

bwplotka · 2019-08-27T16:48:26Z

pkg/store/bucket.go

+func (s *BucketStore) isBlockInMinMaxRange(ctx context.Context, id ulid.ULID) (bool, error) {
+	dir := filepath.Join(s.dir, id.String())
+
+	b := &bucketBlock{


Allocating the whole block for quick check is not great. I think we should just extract loadMeta method to separate function and user.

Also at some point we need some meta syncer/ matcher that deals with all of this, something similar to #1394 ):

Also even with time slicing looks like we will download all meta.json files to disk. Is that what we want? Essentially time slices will not include meta.json files on disk.

Function extracted.

Good question, IMO those meta.json files are small in size, so why not? It will speed up our next meta.json lookup.

pkg/store/bucket.go

bwplotka · 2019-08-27T16:56:29Z

pkg/store/bucket_e2e_test.go

+	defer func() { testutil.Ok(t, os.RemoveAll(dir)) }()
+
+	series := []labels.Labels{
+		labels.FromStrings("a", "1", "b", "1"),


Can we reuse prepareStoreWithTestBlocks? Series looks exactly the same.

I splitted the prepareTestBlocks from prepareStoreWithTestBlocks so that we can easily configure Store and generate test blocks.

I tried to adjust prepareStoreWithTestBlocks, but I didn't like the newly added parameter for filtering and it would need quite a lot of changes due to changes in sync blocks https://github.com/thanos-io/thanos/blob/master/pkg/store/bucket_e2e_test.go#L155-L159

IMO that func is awesome for https://github.com/thanos-io/thanos/blob/master/pkg/store/bucket_e2e_test.go#L365, but doesn't really fit my use case, as I need custom checks in Store gw

d-ulyanov · 2019-08-21T16:10:31Z

pkg/store/bucket.go

@@ -468,6 +514,10 @@ func (s *BucketStore) TimeRange() (mint, maxt int64) {
 			maxt = b.meta.MaxTime
 		}
 	}
+
+	mint = s.normalizeMinTime(mint)


I think that it would be better to put "normalization" code right here, it would be more easy to read

but we do it in 2 places.. I think we just need to rename to something clear.

d-ulyanov · 2019-08-21T16:24:44Z

cmd/thanos/store.go

+	minTime := model.TimeOrDuration(cmd.Flag("min-time", "Start of time range limit to serve. Thanos Store serves only metrics, which happened later than this value. Option can be a constant time in RFC3339 format or time duration relative to current time, such as -1d or 2h45m. Valid duration units are ms, s, m, h, d, w, y.").
+		Default("0000-01-01T00:00:00Z"))
+
+	maxTime := model.TimeOrDuration(cmd.Flag("max-time", "End of time range limit to serve. Thanos Store serves only blocks, which happened eariler than this value. Option can be a constant time in RFC3339 format or time duration relative to current time, such as -1d or 2h45m. Valid duration units are ms, s, m, h, d, w, y.").


We definitely need the same logic for Prometheus sidecar.

My point is that we should avoid fetching duplicated data from different stores when it's possible to decrease merging time and memory usage.

if Prom retention = 2h it doesn't mean that 2h is all the data that Prom will return all the time. Usually, you will see more than 2h in response (up to retention x 2). We can avoid data intersection just adding max-time to queries

Second important thing: we would like to have alerts that use more than 2h of data. There are not a lot of such queries, but we have them. It's a good idea to run these queries on local Prom data, so we need to increase retention up to 7d. But increasing retention we're also increasing memory usage (query cache). Also, increasing retention we're increasing network traffic and finally Thanos Query will merge more data (and we're sure that it's the same data).

I think that makes sense to me.

d-ulyanov

Guys, what do you think about implementing the same in the sidecar?

bwplotka · 2019-08-27T20:52:43Z

Sure, but not for this PR (:

Signed-off-by: Povilas Versockas <p.versockas@gmail.com>

povilasv · 2019-08-28T04:55:12Z

Guys, what do you think about implementing the same in the sidecar?

For Sidecar I think we have a similar PR open #1267

bwplotka

LGTM 👍

Minor questions only.

pkg/store/bucket.go

bwplotka · 2019-08-28T08:36:54Z

pkg/store/bucket_e2e_test.go

+	defer func() { testutil.Ok(t, os.RemoveAll(dir)) }()
+
+	series := []labels.Labels{
+		labels.FromStrings("a", "1", "b", "1"),


bwplotka · 2019-08-28T08:38:26Z

cmd/thanos/store.go

@@ -49,7 +50,18 @@ func registerStore(m map[string]setupFunc, app *kingpin.Application, name string
 	blockSyncConcurrency := cmd.Flag("block-sync-concurrency", "Number of goroutines to use when syncing blocks from object storage.").
 		Default("20").Int()

+	minTime := model.TimeOrDuration(cmd.Flag("min-time", "Start of time range limit to serve. Thanos Store serves only metrics, which happened later than this value. Option can be a constant time in RFC3339 format or time duration relative to current time, such as -1d or 2h45m. Valid duration units are ms, s, m, h, d, w, y.").


Default("0000-01-01T00:00:00Z")) Is that lowest min time? What about -9999-12-31T23:59:59Z ?

t, err = time.Parse(time.RFC3339, "-9999-12-31T23:59:59Z") if err != nil { fmt.Println(err) } fmt.Println(t)

gives:

parsing time "-9999-12-31T23:59:59Z" as "2006-01-02T15:04:05Z07:00": cannot parse "-9999-12-31T23:59:59Z" as "2006"

I would set some explicit default value here (for "" value), but we can do it in next PRs (:

Signed-off-by: Povilas Versockas <p.versockas@gmail.com>

bwplotka

Thanks @povilasv

bwplotka · 2019-08-28T09:21:26Z

cmd/thanos/store.go

@@ -49,7 +50,18 @@ func registerStore(m map[string]setupFunc, app *kingpin.Application, name string
 	blockSyncConcurrency := cmd.Flag("block-sync-concurrency", "Number of goroutines to use when syncing blocks from object storage.").
 		Default("20").Int()

+	minTime := model.TimeOrDuration(cmd.Flag("min-time", "Start of time range limit to serve. Thanos Store serves only metrics, which happened later than this value. Option can be a constant time in RFC3339 format or time duration relative to current time, such as -1d or 2h45m. Valid duration units are ms, s, m, h, d, w, y.").


I would set some explicit default value here (for "" value), but we can do it in next PRs (:

* Store: Add Time & duration based partitioning Signed-off-by: Povilas Versockas <p.versockas@gmail.com>

povilasv force-pushed the time-partitioning2 branch from 23d12ff to 98156da Compare August 13, 2019 15:34

povilasv mentioned this pull request Aug 13, 2019

Store: Add time & duration based partitioning #1077

Closed

povilasv force-pushed the time-partitioning2 branch from 98156da to 01389e8 Compare August 13, 2019 15:41

bwplotka mentioned this pull request Aug 15, 2019

Unified fetching/syncing block metadata everywhere #1394

Closed

povilasv force-pushed the time-partitioning2 branch from 01524da to 9a8a2ae Compare August 27, 2019 10:14

Store: Add Time & duration based partitioning

bed5771

Signed-off-by: Povilas Versockas <p.versockas@gmail.com>

povilasv force-pushed the time-partitioning2 branch from 9a8a2ae to bed5771 Compare August 27, 2019 10:17

povilasv marked this pull request as ready for review August 27, 2019 10:27

Merge branch 'master' into time-partitioning2

2eed935

bwplotka self-requested a review August 27, 2019 16:26

bwplotka reviewed Aug 27, 2019

View reviewed changes

d-ulyanov reviewed Aug 27, 2019

View reviewed changes

Refactor after review

e3c7f06

Signed-off-by: Povilas Versockas <p.versockas@gmail.com>

bwplotka approved these changes Aug 28, 2019

View reviewed changes

Fixes after review

36ba985

Signed-off-by: Povilas Versockas <p.versockas@gmail.com>

bwplotka approved these changes Aug 28, 2019

View reviewed changes

bwplotka merged commit 8a6a70f into master Aug 28, 2019

bwplotka deleted the time-partitioning2 branch August 28, 2019 09:22

claytono mentioned this pull request Aug 28, 2019

compactor - "write postings: exceeding max size of 64GiB" #1424

Closed

wbh1 pushed a commit to wbh1/thanos that referenced this pull request Sep 17, 2019

Store: Add Time & duration based partitioning (thanos-io#1408)

9929990

* Store: Add Time & duration based partitioning Signed-off-by: Povilas Versockas <p.versockas@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store: Add Time & duration based partitioning #1408

Store: Add Time & duration based partitioning #1408

povilasv commented Aug 13, 2019 •

edited

Loading

bwplotka left a comment

bwplotka Aug 27, 2019

bwplotka Aug 27, 2019

povilasv Aug 28, 2019 •

edited

Loading

bwplotka Aug 27, 2019

povilasv Aug 28, 2019

bwplotka Aug 28, 2019

d-ulyanov Aug 21, 2019

bwplotka Aug 27, 2019

d-ulyanov Aug 21, 2019

bwplotka Aug 27, 2019

d-ulyanov left a comment

bwplotka commented Aug 27, 2019

povilasv commented Aug 28, 2019

bwplotka left a comment

bwplotka Aug 28, 2019

bwplotka Aug 28, 2019

povilasv Aug 28, 2019

bwplotka Aug 28, 2019

bwplotka left a comment

bwplotka Aug 28, 2019

Store: Add Time & duration based partitioning #1408

Store: Add Time & duration based partitioning #1408

Conversation

povilasv commented Aug 13, 2019 • edited Loading

Changes

Verification

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

povilasv Aug 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-ulyanov left a comment

Choose a reason for hiding this comment

bwplotka commented Aug 27, 2019

povilasv commented Aug 28, 2019

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

povilasv commented Aug 13, 2019 •

edited

Loading

povilasv Aug 28, 2019 •

edited

Loading