Compaction implementation for block-ranges #103

gouthamve · 2017-07-06T19:36:44Z

Super early implementation:

So if you have 20, 60, 240, 720, 2160 as your ranges, you pick the oldest 2160 block you can compact and inside that, you pick the oldest 720 block and so on.

TODO:

More testing.
Simplification and more comments.
Optimise some parts.
Choose the ranges by retention.
Align ranges by parent instead of 0 aligning all ranges.
Cut blocks properly.

/cc @fabxc

This change is

gouthamve · 2017-07-06T19:37:18Z

Fixes #102

gouthamve · 2017-07-07T11:53:14Z

I just pushed a commit which some very strong defaults:

The min-block-size is 2h or lower not more than that.
We were thinking about doing 0 aligning of the largest block and every other block aligned to the parent. By making sure that every block-dur cleanly divides the parent, we can 0 align all the blocks and still be safe.
We will choose the max block duration so that we don't store data 20% older than retention.

The repercussions for these are the min block duration is 2h or something that divides 2h cleanly which is kinda weird but seems okay.

/cc @fabxc @brian-brazil

Signed-off-by: Goutham Veeramachaneni <cs14btech11014@iith.ac.in>

fabxc

Just a first pass on the the configuration of it all. Will go over the selection implementation next.

fabxc · 2017-07-10T08:59:39Z

compact_test.go

@@ -0,0 +1,232 @@
+package tsdb


License header missing.

fabxc · 2017-07-10T11:51:48Z

compact.go

+	int64(24 * time.Hour),
+	int64(72 * time.Hour),  // 3d
+	int64(216 * time.Hour), // 9d
+}


From a TSDB perspective defaults probably don't make sense as we don't know the timestamp unit and the sample interval. Maybe a helper function here would we best to which we can give

min size

steps

blocks per step

For example in prometheus: ExponentialBlockRanges(2 * time.Hour / time.Millisecond, 5, 3)

fabxc · 2017-07-10T11:52:36Z

compact.go

 }

 func newCompactor(dir string, r prometheus.Registerer, l log.Logger, opts *compactorOptions) *compactor {
+	if opts.blockRanges == nil {
+		opts.blockRanges = defaultBlockRanges
+	}


As per above defaults are not generally applicable, just not compacting seems like the right choice here.

Do we need sanity checks on startup that the ranges actually fit within each other?
What happens if the chosen ranges do not work with already existing blocks?

What happens if the chosen ranges do not work with already existing blocks?

Some blocks which cross the boundaries of the max-block-range would never be compacted. Which is okay, I guess.

fabxc · 2017-07-10T12:15:00Z

db.go

@@ -60,11 +59,9 @@ type Options struct {

 	// The timestamp range of head blocks after which they get persisted.
 	// It's the minimum duration of any persisted block.
+	// It should divide 2h cleanly and should be less than or equal to 2h. Default 2h.
 	MinBlockDuration uint64


As per above suggestion, we probably want to pass in the full ranges as config options instead of this value. The first range is than equivalent to MinBlockDuration. As said there, we also shouldn't make assumptions about stuff like 2h.

At least for bench/debugging purposes it probably would be a good idea to still allow passing in short initial ranges via Prometheus. Maybe a hidden --debug.storage.tsdb... flag?

fabxc · 2017-07-10T12:17:53Z

db.go

+		if float64(copts.blockRanges[len(copts.blockRanges)-1])/float64(opts.RetentionDuration) > 0.2 {
+			copts.blockRanges = copts.blockRanges[:len(copts.blockRanges)-1]
+		}
+	}


This would just spin if the highest range is less than 20% of the retention, no?

gouthamve · 2017-07-11T09:49:15Z

db.go

+		return nil, errors.New("atleast one block-range should exist.")
+	}
+
+	for float64(copts.blockRanges[len(copts.blockRanges)-1])/float64(opts.RetentionDuration) > 0.2 {


@fabxc Given we allow passing of full ranges, should we still do this?

Yea, I think so. Imagine we generally have a hard-coded set of ranges. The maximum one we actually compact to should still be ceiled by the dynamically configured retention.

fabxc · 2017-07-12T13:56:20Z

compact.go

+}
+
+func (c *compactor) selectDirs(ds []dirMeta) []dirMeta {
+	// The way tp skip compaction is to not have blockRanges.


We probably still want this later on to ensure we re-compact stuff with lots of tombstones.
Probably part of #91 – sorry about that lingering around for so long without review.

fabxc · 2017-07-12T14:04:05Z

compact.go

 }

-func (c *compactor) match(dirs []dirMeta) bool {
-	g := dirs[0].meta.Compaction.Generation
+func splitByRange(ds []dirMeta, tr int64) [][]dirMeta {


This function definitely needs heading and inlined docs.

fabxc · 2017-07-12T14:22:11Z

compact.go

+			t0 = dir.meta.MinTime - dir.meta.MinTime%tr
+			if intervalContains(t0, t0+tr, dir.meta.MinTime) && intervalContains(t0, t0+tr, dir.meta.MaxTime) {
+				dirs = append(dirs, dir)
+			}
 		}


Repeating initialization and the condition at different indentation levels makes is hard to grasp.

var splitDirs [][]dirMeta for i := 0; i < len(ds); { var group []dirMeta // Compute start of aligned time range of size tr closest to the current block's start. t0 := ds[i].meta.MinTime - (ds[i].meta.MinTime % tr) // Add all dirs to the current group that are within [t0, t0+tr]. for ;i < len(ds); i++ { if ds[i].meta.MinTime < t0 || ds[i].meta.MaxTime > t0+tr { break } group = append(group, ds[i]) } if len(group) > 0 { splitDirs = append(splitDirs, group) } } return splitDirs

Haven't verified this one, but should roughly do the same.

fabxc · 2017-07-12T14:23:54Z

compact.go

+	dirs := []dirMeta{}
+	for i := len(blocks) - 1; i >= 0; i-- {
+		// We need to choose the oldest blocks to compact. If there are a couple of blocks in
+		// the largest interval, we should compact those first.


We are reverse-iterating. Doesn't that make us chose the youngest group of compactable blocks?

Nope. So we actually sort the dirs in ascending order here: https://github.com/Gouthamve/tsdb/blob/block-ranges/compact.go#L144-L146 hence it makes sense to reverse-iterate.

fabxc · 2017-07-12T14:24:06Z

db.go

+	}
+
+	if len(copts.blockRanges) == 0 {
+		return nil, errors.New("atleast one block-range should exist.")


at least

must exist

No . in errors.

fabxc · 2017-07-12T14:28:04Z

compact.go

+	// If there are too many blocks, see if a smaller interval will catch them.
+	// i.e, if we have 0-20, 60-80, 80-100; all fall under 0-240, but we'd rather compact 60-100
+	// than all at once.
+	if len(dirs) > 2 {


Before we were always compacting groups of 3. Why are we now searching in smaller ranges if we have 3?

gouthamve · 2017-07-12T15:14:55Z

@fabxc Incorporated all the changes. PTAL.

fabxc · 2017-07-12T15:18:26Z

Travis is failing.

fabxc · 2017-07-12T15:22:35Z

👍 if manual testing also confirmed this. Probably good to spin up prombench and let it run for a bit after the changes have been pulled there.

We might want to introduce debug flags to manually set the range so we can run tests with frequent compactions.
Or we even keep min block duration around for high frequency cases... they probably still exist and will run out of memory otherwise.

Signed-off-by: Goutham Veeramachaneni <cs14btech11014@iith.ac.in>

gouthamve requested a review from fabxc July 6, 2017 19:36

gouthamve mentioned this pull request Jul 6, 2017

Tests depend on Prometheus #104

Closed

gouthamve force-pushed the block-ranges branch from 3450a3e to 0942925 Compare July 7, 2017 10:15

Initial compaction implementation for block-ranges

ce23adb

Signed-off-by: Goutham Veeramachaneni <cs14btech11014@iith.ac.in>

gouthamve force-pushed the block-ranges branch from 46f91ca to 663132b Compare July 7, 2017 14:33

fabxc reviewed Jul 10, 2017

View reviewed changes

gouthamve force-pushed the block-ranges branch 2 times, most recently from 74cb7e0 to 6a7e329 Compare July 11, 2017 09:48

gouthamve commented Jul 11, 2017

View reviewed changes

fabxc reviewed Jul 12, 2017

View reviewed changes

gouthamve force-pushed the block-ranges branch from 6a7e329 to 80b0bea Compare July 12, 2017 15:14

gouthamve changed the title ~~[WIP] compaction implementation for block-ranges~~ Compaction implementation for block-ranges Jul 12, 2017

Choose ranges by retention and min-block-duration

9a2ab73

Signed-off-by: Goutham Veeramachaneni <cs14btech11014@iith.ac.in>

gouthamve force-pushed the block-ranges branch from 80b0bea to 9a2ab73 Compare July 12, 2017 15:30

gouthamve merged commit 7b9c536 into prometheus-junkyard:master Jul 12, 2017

gouthamve mentioned this pull request Oct 3, 2017

Fix block ranges #102

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compaction implementation for block-ranges #103

Compaction implementation for block-ranges #103

gouthamve commented Jul 6, 2017 •

edited

gouthamve commented Jul 6, 2017

gouthamve commented Jul 7, 2017

fabxc left a comment

fabxc Jul 10, 2017

fabxc Jul 10, 2017

fabxc Jul 10, 2017

fabxc Jul 10, 2017

gouthamve Jul 10, 2017

fabxc Jul 10, 2017

fabxc Jul 10, 2017

gouthamve Jul 11, 2017 •

edited

fabxc Jul 11, 2017

fabxc Jul 12, 2017

fabxc Jul 12, 2017

fabxc Jul 12, 2017

fabxc Jul 12, 2017

fabxc Jul 12, 2017

gouthamve Jul 12, 2017

fabxc Jul 12, 2017

fabxc Jul 12, 2017

gouthamve commented Jul 12, 2017

fabxc commented Jul 12, 2017

fabxc commented Jul 12, 2017

Compaction implementation for block-ranges #103

Compaction implementation for block-ranges #103

Conversation

gouthamve commented Jul 6, 2017 • edited

gouthamve commented Jul 6, 2017

gouthamve commented Jul 7, 2017

fabxc 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gouthamve Jul 11, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gouthamve commented Jul 12, 2017

fabxc commented Jul 12, 2017

fabxc commented Jul 12, 2017

gouthamve commented Jul 6, 2017 •

edited

gouthamve Jul 11, 2017 •

edited