From 653f3435470407d973f342fbecccb826ea767264 Mon Sep 17 00:00:00 2001 From: Julien Pivotto Date: Wed, 25 Mar 2020 20:13:47 +0100 Subject: [PATCH 1/3] Revert head posting optimization This reverts commit 52630ad0c735f2dce4ce5bb851acb6c5d7df5eb1. Signed-off-by: Julien Pivotto --- tsdb/block.go | 7 ++- tsdb/cmd/tsdb/main.go | 2 +- tsdb/compact.go | 2 +- tsdb/compact_test.go | 12 ++--- tsdb/db_test.go | 8 ++-- tsdb/head.go | 47 +++----------------- tsdb/head_test.go | 90 +------------------------------------- tsdb/mocks_test.go | 4 +- tsdb/querier.go | 2 +- tsdb/querier_bench_test.go | 5 +-- tsdb/querier_test.go | 4 +- tsdb/wal/wal_test.go | 2 +- 12 files changed, 30 insertions(+), 155 deletions(-) diff --git a/tsdb/block.go b/tsdb/block.go index bcdec5c88..2d26f9038 100644 --- a/tsdb/block.go +++ b/tsdb/block.go @@ -111,9 +111,8 @@ type ChunkReader interface { // BlockReader provides reading access to a data block. type BlockReader interface { - // Index returns an IndexReader over the block's data within the specified - // timeframe. - Index(mint, maxt int64) (IndexReader, error) + // Index returns an IndexReader over the block's data. + Index() (IndexReader, error) // Chunks returns a ChunkReader over the block's data. Chunks() (ChunkReader, error) @@ -373,7 +372,7 @@ func (pb *Block) startRead() error { } // Index returns a new IndexReader against the block data. -func (pb *Block) Index(mint, maxt int64) (IndexReader, error) { +func (pb *Block) Index() (IndexReader, error) { if err := pb.startRead(); err != nil { return nil, err } diff --git a/tsdb/cmd/tsdb/main.go b/tsdb/cmd/tsdb/main.go index 3b2ebf296..1c4132236 100644 --- a/tsdb/cmd/tsdb/main.go +++ b/tsdb/cmd/tsdb/main.go @@ -471,7 +471,7 @@ func analyzeBlock(b tsdb.BlockReader, limit int) error { // Presume 1ms resolution that Prometheus uses. fmt.Printf("Duration: %s\n", (time.Duration(meta.MaxTime-meta.MinTime) * 1e6).String()) fmt.Printf("Series: %d\n", meta.Stats.NumSeries) - ir, err := b.Index(math.MinInt64, math.MaxInt64) + ir, err := b.Index() if err != nil { return err } diff --git a/tsdb/compact.go b/tsdb/compact.go index 70aa9eee0..9eb64439d 100644 --- a/tsdb/compact.go +++ b/tsdb/compact.go @@ -681,7 +681,7 @@ func (c *LeveledCompactor) populateBlock(blocks []BlockReader, meta *BlockMeta, } } - indexr, err := b.Index(math.MinInt64, globalMaxt) + indexr, err := b.Index() if err != nil { return errors.Wrapf(err, "open index reader for block %s", b) } diff --git a/tsdb/compact_test.go b/tsdb/compact_test.go index a0a9d7a38..31734a574 100644 --- a/tsdb/compact_test.go +++ b/tsdb/compact_test.go @@ -456,10 +456,10 @@ func metaRange(name string, mint, maxt int64, stats *BlockStats) dirMeta { type erringBReader struct{} -func (erringBReader) Index(int64, int64) (IndexReader, error) { return nil, errors.New("index") } -func (erringBReader) Chunks() (ChunkReader, error) { return nil, errors.New("chunks") } -func (erringBReader) Tombstones() (tombstones.Reader, error) { return nil, errors.New("tombstones") } -func (erringBReader) Meta() BlockMeta { return BlockMeta{} } +func (erringBReader) Index() (IndexReader, error) { return nil, errors.New("index") } +func (erringBReader) Chunks() (ChunkReader, error) { return nil, errors.New("chunks") } +func (erringBReader) Tombstones() (tombstones.Reader, error) { return nil, errors.New("tombstones") } +func (erringBReader) Meta() BlockMeta { return BlockMeta{} } type nopChunkWriter struct{} @@ -652,7 +652,7 @@ func TestCompaction_populateBlock(t *testing.T) { expErr: errors.New("found chunk with minTime: 10 maxTime: 30 outside of compacted minTime: 0 maxTime: 20"), }, { - // Introduced by https://github.com/prometheus/tsdb/issues/347. + // Introduced by https://github.com/prometheus/prometheus/tsdb/issues/347. title: "Populate from single block containing extra chunk", inputSeriesSamples: [][]seriesSamples{ { @@ -692,7 +692,7 @@ func TestCompaction_populateBlock(t *testing.T) { }, }, { - // Introduced by https://github.com/prometheus/tsdb/pull/539. + // Introduced by https://github.com/prometheus/prometheus/tsdb/pull/539. title: "Populate from three blocks that the last two are overlapping.", inputSeriesSamples: [][]seriesSamples{ { diff --git a/tsdb/db_test.go b/tsdb/db_test.go index 545473f82..0b06643aa 100644 --- a/tsdb/db_test.go +++ b/tsdb/db_test.go @@ -1410,7 +1410,7 @@ func TestOverlappingBlocksDetectsAllOverlaps(t *testing.T) { }, OverlappingBlocks(nc1)) } -// Regression test for https://github.com/prometheus/tsdb/issues/347 +// Regression test for https://github.com/prometheus/prometheus/tsdb/issues/347 func TestChunkAtBlockBoundary(t *testing.T) { db, closeFn := openTestDB(t, nil, nil) defer func() { @@ -1437,7 +1437,7 @@ func TestChunkAtBlockBoundary(t *testing.T) { testutil.Ok(t, err) for _, block := range db.Blocks() { - r, err := block.Index(math.MinInt64, math.MaxInt64) + r, err := block.Index() testutil.Ok(t, err) defer r.Close() @@ -1787,7 +1787,7 @@ func TestDB_LabelNames(t *testing.T) { appendSamples(db, 0, 4, tst.sampleLabels1) // Testing head. - headIndexr, err := db.head.Index(math.MinInt64, math.MaxInt64) + headIndexr, err := db.head.Index() testutil.Ok(t, err) labelNames, err := headIndexr.LabelNames() testutil.Ok(t, err) @@ -1800,7 +1800,7 @@ func TestDB_LabelNames(t *testing.T) { // All blocks have same label names, hence check them individually. // No need to aggregate and check. for _, b := range db.Blocks() { - blockIndexr, err := b.Index(math.MinInt64, math.MaxInt64) + blockIndexr, err := b.Index() testutil.Ok(t, err) labelNames, err = blockIndexr.LabelNames() testutil.Ok(t, err) diff --git a/tsdb/head.go b/tsdb/head.go index bbdfead9d..8151b7ca5 100644 --- a/tsdb/head.go +++ b/tsdb/head.go @@ -754,7 +754,7 @@ type RangeHead struct { mint, maxt int64 } -// NewRangeHead returns a *RangeHead. +// NewRangeHead returns a *rangeHead. func NewRangeHead(head *Head, mint, maxt int64) *RangeHead { return &RangeHead{ head: head, @@ -763,15 +763,8 @@ func NewRangeHead(head *Head, mint, maxt int64) *RangeHead { } } -func (h *RangeHead) Index(mint, maxt int64) (IndexReader, error) { - // rangeHead guarantees that the series returned are within its range. - if mint < h.mint { - mint = h.mint - } - if maxt > h.maxt { - maxt = h.maxt - } - return h.head.indexRange(mint, maxt), nil +func (h *RangeHead) Index() (IndexReader, error) { + return h.head.indexRange(h.mint, h.maxt), nil } func (h *RangeHead) Chunks() (ChunkReader, error) { @@ -1196,8 +1189,8 @@ func (h *Head) Tombstones() (tombstones.Reader, error) { } // Index returns an IndexReader against the block. -func (h *Head) Index(mint, maxt int64) (IndexReader, error) { - return h.indexRange(mint, maxt), nil +func (h *Head) Index() (IndexReader, error) { + return h.indexRange(math.MinInt64, math.MaxInt64), nil } func (h *Head) indexRange(mint, maxt int64) *headIndexReader { @@ -1390,37 +1383,9 @@ func (h *headIndexReader) LabelNames() ([]string, error) { // Postings returns the postings list iterator for the label pairs. func (h *headIndexReader) Postings(name string, values ...string) (index.Postings, error) { - fullRange := h.mint <= h.head.MinTime() && h.maxt >= h.head.MaxTime() res := make([]index.Postings, 0, len(values)) for _, value := range values { - p := h.head.postings.Get(name, value) - if fullRange { - // The head timerange covers the full index reader timerange. - // All the series can the be appended without filtering. - res = append(res, p) - continue - } - - // Filter out series not in the time range, to avoid - // later on building up all the chunk metadata just to - // discard it. - filtered := []uint64{} - for p.Next() { - s := h.head.series.getByID(p.At()) - if s == nil { - level.Debug(h.head.logger).Log("msg", "looked up series not found") - continue - } - s.RLock() - if s.minTime() <= h.maxt && s.maxTime() >= h.mint { - filtered = append(filtered, p.At()) - } - s.RUnlock() - } - if p.Err() != nil { - return nil, p.Err() - } - res = append(res, index.NewListPostings(filtered)) + res = append(res, h.head.postings.Get(name, value)) } return index.Merge(res...), nil } diff --git a/tsdb/head_test.go b/tsdb/head_test.go index 57e5c3a92..08c53418d 100644 --- a/tsdb/head_test.go +++ b/tsdb/head_test.go @@ -1333,94 +1333,6 @@ func TestAddDuplicateLabelName(t *testing.T) { add(labels.Labels{{Name: "__name__", Value: "up"}, {Name: "job", Value: "prometheus"}, {Name: "le", Value: "500"}, {Name: "le", Value: "400"}, {Name: "unit", Value: "s"}}, "le") } -func TestHeadSeriesWithTimeBoundaries(t *testing.T) { - h, err := NewHead(nil, nil, nil, 15, DefaultStripeSize) - testutil.Ok(t, err) - defer h.Close() - testutil.Ok(t, h.Init(0)) - app := h.Appender() - - s1, err := app.Add(labels.FromStrings("foo1", "bar"), 2, 0) - testutil.Ok(t, err) - for ts := int64(3); ts < 13; ts++ { - err = app.AddFast(s1, ts, 0) - testutil.Ok(t, err) - } - s2, err := app.Add(labels.FromStrings("foo2", "bar"), 5, 0) - testutil.Ok(t, err) - for ts := int64(6); ts < 11; ts++ { - err = app.AddFast(s2, ts, 0) - testutil.Ok(t, err) - } - s3, err := app.Add(labels.FromStrings("foo3", "bar"), 5, 0) - testutil.Ok(t, err) - err = app.AddFast(s3, 6, 0) - testutil.Ok(t, err) - _, err = app.Add(labels.FromStrings("foo4", "bar"), 9, 0) - testutil.Ok(t, err) - - testutil.Ok(t, app.Commit()) - - cases := []struct { - mint int64 - maxt int64 - seriesCount int - samplesCount int - }{ - // foo1 ..00000000000.. - // foo2 .....000000.... - // foo3 .....00........ - // foo4 .........0..... - {mint: 0, maxt: 0, seriesCount: 0, samplesCount: 0}, - {mint: 0, maxt: 1, seriesCount: 0, samplesCount: 0}, - {mint: 0, maxt: 2, seriesCount: 1, samplesCount: 1}, - {mint: 2, maxt: 2, seriesCount: 1, samplesCount: 1}, - {mint: 0, maxt: 4, seriesCount: 1, samplesCount: 3}, - {mint: 0, maxt: 5, seriesCount: 3, samplesCount: 6}, - {mint: 0, maxt: 6, seriesCount: 3, samplesCount: 9}, - {mint: 0, maxt: 7, seriesCount: 3, samplesCount: 11}, - {mint: 0, maxt: 8, seriesCount: 3, samplesCount: 13}, - {mint: 0, maxt: 9, seriesCount: 4, samplesCount: 16}, - {mint: 0, maxt: 10, seriesCount: 4, samplesCount: 18}, - {mint: 0, maxt: 11, seriesCount: 4, samplesCount: 19}, - {mint: 0, maxt: 12, seriesCount: 4, samplesCount: 20}, - {mint: 0, maxt: 13, seriesCount: 4, samplesCount: 20}, - {mint: 0, maxt: 14, seriesCount: 4, samplesCount: 20}, - {mint: 2, maxt: 14, seriesCount: 4, samplesCount: 20}, - {mint: 3, maxt: 14, seriesCount: 4, samplesCount: 19}, - {mint: 4, maxt: 14, seriesCount: 4, samplesCount: 18}, - {mint: 8, maxt: 9, seriesCount: 3, samplesCount: 5}, - {mint: 9, maxt: 9, seriesCount: 3, samplesCount: 3}, - {mint: 6, maxt: 9, seriesCount: 4, samplesCount: 10}, - {mint: 11, maxt: 11, seriesCount: 1, samplesCount: 1}, - {mint: 11, maxt: 12, seriesCount: 1, samplesCount: 2}, - {mint: 11, maxt: 14, seriesCount: 1, samplesCount: 2}, - {mint: 12, maxt: 14, seriesCount: 1, samplesCount: 1}, - } - - for i, c := range cases { - matcher := labels.MustNewMatcher(labels.MatchEqual, "", "") - q, err := NewBlockQuerier(h, c.mint, c.maxt) - testutil.Ok(t, err) - - seriesCount := 0 - samplesCount := 0 - ss, _, err := q.Select(false, nil, matcher) - testutil.Ok(t, err) - for ss.Next() { - i := ss.At().Iterator() - for i.Next() { - samplesCount++ - } - seriesCount++ - } - testutil.Ok(t, ss.Err()) - testutil.Equals(t, c.seriesCount, seriesCount, "test series %d", i) - testutil.Equals(t, c.samplesCount, samplesCount, "test samples %d", i) - q.Close() - } -} - func TestMemSeriesIsolation(t *testing.T) { // Put a series, select it. GC it and then access it. hb, err := NewHead(nil, nil, nil, 1000, DefaultStripeSize) @@ -1428,7 +1340,7 @@ func TestMemSeriesIsolation(t *testing.T) { defer hb.Close() lastValue := func(maxAppendID uint64) int { - idx, err := hb.Index(hb.MinTime(), hb.MaxTime()) + idx, err := hb.Index() testutil.Ok(t, err) iso := hb.iso.State() diff --git a/tsdb/mocks_test.go b/tsdb/mocks_test.go index a4e47b8cb..913061e83 100644 --- a/tsdb/mocks_test.go +++ b/tsdb/mocks_test.go @@ -71,8 +71,8 @@ type mockBReader struct { maxt int64 } -func (r *mockBReader) Index(mint, maxt int64) (IndexReader, error) { return r.ir, nil } -func (r *mockBReader) Chunks() (ChunkReader, error) { return r.cr, nil } +func (r *mockBReader) Index() (IndexReader, error) { return r.ir, nil } +func (r *mockBReader) Chunks() (ChunkReader, error) { return r.cr, nil } func (r *mockBReader) Tombstones() (tombstones.Reader, error) { return tombstones.NewMemTombstones(), nil } diff --git a/tsdb/querier.go b/tsdb/querier.go index ea1100a52..39fc98141 100644 --- a/tsdb/querier.go +++ b/tsdb/querier.go @@ -154,7 +154,7 @@ func (q *verticalQuerier) sel(sortSeries bool, hints *storage.SelectHints, qs [] // NewBlockQuerier returns a querier against the reader. func NewBlockQuerier(b BlockReader, mint, maxt int64) (storage.Querier, error) { - indexr, err := b.Index(mint, maxt) + indexr, err := b.Index() if err != nil { return nil, errors.Wrapf(err, "open index reader") } diff --git a/tsdb/querier_bench_test.go b/tsdb/querier_bench_test.go index 95502be95..3d0a3fbcf 100644 --- a/tsdb/querier_bench_test.go +++ b/tsdb/querier_bench_test.go @@ -16,7 +16,6 @@ package tsdb import ( "fmt" "io/ioutil" - "math" "os" "strconv" "testing" @@ -54,7 +53,7 @@ func BenchmarkPostingsForMatchers(b *testing.B) { } testutil.Ok(b, app.Commit()) - ir, err := h.Index(math.MinInt64, math.MaxInt64) + ir, err := h.Index() testutil.Ok(b, err) b.Run("Head", func(b *testing.B) { benchmarkPostingsForMatchers(b, ir) @@ -72,7 +71,7 @@ func BenchmarkPostingsForMatchers(b *testing.B) { defer func() { testutil.Ok(b, block.Close()) }() - ir, err = block.Index(math.MinInt64, math.MaxInt64) + ir, err = block.Index() testutil.Ok(b, err) defer ir.Close() b.Run("Block", func(b *testing.B) { diff --git a/tsdb/querier_test.go b/tsdb/querier_test.go index 9668a21fe..536ed2dff 100644 --- a/tsdb/querier_test.go +++ b/tsdb/querier_test.go @@ -1044,7 +1044,7 @@ func TestSeriesIterator(t *testing.T) { }) } -// Regression for: https://github.com/prometheus/tsdb/pull/97 +// Regression for: https://github.com/prometheus/prometheus/tsdb/pull/97 func TestChunkSeriesIterator_DoubleSeek(t *testing.T) { chkMetas := []chunks.Meta{ tsdbutil.ChunkFromSamples([]tsdbutil.Sample{}), @@ -2137,7 +2137,7 @@ func TestPostingsForMatchers(t *testing.T) { }, } - ir, err := h.Index(math.MinInt64, math.MaxInt64) + ir, err := h.Index() testutil.Ok(t, err) for _, c := range cases { diff --git a/tsdb/wal/wal_test.go b/tsdb/wal/wal_test.go index 340c2b32e..cd7a9fd8f 100644 --- a/tsdb/wal/wal_test.go +++ b/tsdb/wal/wal_test.go @@ -49,7 +49,7 @@ func TestWALRepair_ReadingError(t *testing.T) { }, // Ensures that the page buffer is big enough to fit // an entire page size without panicking. - // https://github.com/prometheus/tsdb/pull/414 + // https://github.com/prometheus/prometheus/tsdb/pull/414 "bad_header": { 1, func(f *os.File) { From 73228b1b6855007a92d0daf6ef4823cb5b8da08a Mon Sep 17 00:00:00 2001 From: Julien Pivotto Date: Wed, 25 Mar 2020 20:26:10 +0100 Subject: [PATCH 2/3] Those links should not be reverted Signed-off-by: Julien Pivotto --- tsdb/compact_test.go | 4 ++-- tsdb/db_test.go | 2 +- tsdb/querier_test.go | 2 +- tsdb/wal/wal_test.go | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tsdb/compact_test.go b/tsdb/compact_test.go index 31734a574..3b72fe02c 100644 --- a/tsdb/compact_test.go +++ b/tsdb/compact_test.go @@ -652,7 +652,7 @@ func TestCompaction_populateBlock(t *testing.T) { expErr: errors.New("found chunk with minTime: 10 maxTime: 30 outside of compacted minTime: 0 maxTime: 20"), }, { - // Introduced by https://github.com/prometheus/prometheus/tsdb/issues/347. + // Introduced by https://github.com/prometheus/tsdb/issues/347. title: "Populate from single block containing extra chunk", inputSeriesSamples: [][]seriesSamples{ { @@ -692,7 +692,7 @@ func TestCompaction_populateBlock(t *testing.T) { }, }, { - // Introduced by https://github.com/prometheus/prometheus/tsdb/pull/539. + // Introduced by https://github.com/prometheus/tsdb/pull/539. title: "Populate from three blocks that the last two are overlapping.", inputSeriesSamples: [][]seriesSamples{ { diff --git a/tsdb/db_test.go b/tsdb/db_test.go index 0b06643aa..29b2bdbea 100644 --- a/tsdb/db_test.go +++ b/tsdb/db_test.go @@ -1410,7 +1410,7 @@ func TestOverlappingBlocksDetectsAllOverlaps(t *testing.T) { }, OverlappingBlocks(nc1)) } -// Regression test for https://github.com/prometheus/prometheus/tsdb/issues/347 +// Regression test for https://github.com/prometheus/tsdb/issues/347 func TestChunkAtBlockBoundary(t *testing.T) { db, closeFn := openTestDB(t, nil, nil) defer func() { diff --git a/tsdb/querier_test.go b/tsdb/querier_test.go index 536ed2dff..241b430b0 100644 --- a/tsdb/querier_test.go +++ b/tsdb/querier_test.go @@ -1044,7 +1044,7 @@ func TestSeriesIterator(t *testing.T) { }) } -// Regression for: https://github.com/prometheus/prometheus/tsdb/pull/97 +// Regression for: https://github.com/prometheus/tsdb/pull/97 func TestChunkSeriesIterator_DoubleSeek(t *testing.T) { chkMetas := []chunks.Meta{ tsdbutil.ChunkFromSamples([]tsdbutil.Sample{}), diff --git a/tsdb/wal/wal_test.go b/tsdb/wal/wal_test.go index cd7a9fd8f..340c2b32e 100644 --- a/tsdb/wal/wal_test.go +++ b/tsdb/wal/wal_test.go @@ -49,7 +49,7 @@ func TestWALRepair_ReadingError(t *testing.T) { }, // Ensures that the page buffer is big enough to fit // an entire page size without panicking. - // https://github.com/prometheus/prometheus/tsdb/pull/414 + // https://github.com/prometheus/tsdb/pull/414 "bad_header": { 1, func(f *os.File) { From ceef10cee4aee55002e525224ba645762abb1de7 Mon Sep 17 00:00:00 2001 From: Julien Pivotto Date: Thu, 26 Mar 2020 00:17:56 +0100 Subject: [PATCH 3/3] Reset comment Signed-off-by: Julien Pivotto --- tsdb/head.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tsdb/head.go b/tsdb/head.go index 8151b7ca5..b8494f618 100644 --- a/tsdb/head.go +++ b/tsdb/head.go @@ -754,7 +754,7 @@ type RangeHead struct { mint, maxt int64 } -// NewRangeHead returns a *rangeHead. +// NewRangeHead returns a *RangeHead. func NewRangeHead(head *Head, mint, maxt int64) *RangeHead { return &RangeHead{ head: head,