Don't write empty blocks #374

codesome · 2018-09-08T03:04:31Z

Fixes #309
Supersedes #245

compact.go

Signed-off-by: Ganesh Vernekar <[email protected]>

krasi-georgiev · 2018-09-11T06:55:12Z

LGTM

krasi-georgiev · 2018-09-11T11:16:52Z

actually I just thought it will be good to add a test for this with some comments why we don't want empty blocks.

gouthamve · 2018-09-11T12:14:47Z

Yup, a test for this would be good. LGTM otherwise.

codesome · 2018-09-11T18:08:40Z

And the test found out a bug. The compaction of head was going on in an infinite loop, but not sure why.

One workaround that I tested (which worked properly) was that, send the dir of empty block in Plan(). And in compaction delete the blocks which are empty.

Does this sound fine?

krasi-georgiev · 2018-09-11T19:04:45Z

can you add the test so I can check what is happening.

Signed-off-by: Ganesh Vernekar <[email protected]>

codesome · 2018-09-12T06:03:22Z

@krasi-georgiev I added the test case.

krasi-georgiev · 2018-09-12T11:23:00Z

https://github.com/prometheus/tsdb/blob/2945db18cafdef58d9c511d4c9f223becb69ac5a/db.go#L374-L376

it never exits the loop because the head range is not reset.
so either

when adding empty sample the head should not change the min/maxtime
or
when meta.Stats.NumSamples == 0 should also reset the head min/maxtime.

I think 1 makes more sense.

gouthamve · 2018-09-12T11:50:51Z

when adding empty sample the head should not change the min/maxtime

We never add "empty" sample :) We can delete samples though. It's kinda hard to set the mintime during delete.

krasi-georgiev · 2018-09-17T17:05:49Z

@codesome still stuck with the test?

codesome · 2018-09-17T17:07:54Z

@krasi-georgiev Stuck with exams 😛. I will be back on it tomorrow.

krasi-georgiev · 2018-09-17T17:09:07Z

Thanks!

codesome · 2018-09-18T09:02:45Z

EDIT: You can skip this comment. Not relevant anymore.

@gouthamve @krasi-georgiev
The thing that is broken in this PR feels not straight forward to fix (will dig in more), so I have come up with this alternative approach.

https://github.com/prometheus/tsdb/compare/master...codesome:remove-empty-block-in-compact?expand=1

This allows you to write empty block during compact, but removes it immediately after. More like, empty blocks might appear during compact(), but not after it has finished. How does this look?

krasi-georgiev · 2018-09-18T12:21:09Z

I would say lets spend more time in trying to fix the test.

ping me if you run out if ideas and I can also have a look.

codesome · 2018-09-18T12:43:53Z

@krasi-georgiev Actually the test is not broken, the code is broken. I actually found a fix for it (tested, working), I will push it soon.

When number of blocks becomes 0, head is not truncated in reload(). This fixes it. Signed-off-by: Ganesh Vernekar <[email protected]>

db_test.go

With previous design if a block is not created from parent blocks, the parent block gets undeleted. This adds `Deletable` field in `BlockMetaCompaction` which helps find such parent blocks. Signed-off-by: Ganesh Vernekar <[email protected]>

codesome · 2018-09-19T07:16:27Z

@krasi-georgiev This is good for anther review.
/cc @gouthamve

compact.go

db.go

db_test.go

db.go

db_test.go

krasi-georgiev · 2018-09-19T11:19:49Z

lets also update the comment for the Compactor interface

// Compact runs compaction against the provided directories. Must
// only be called concurrently with results of Plan().
// Compaction resulting in an empty block marks all parents as deletable in its meta data..
Compact(dest string, dirs ...string) (ulid.ULID, error)
``

db.go

block.go

Signed-off-by: Ganesh Vernekar <[email protected]>

Signed-off-by: Krasi Georgiev <[email protected]>

db_test.go

Pull/374 review

Signed-off-by: Ganesh Vernekar <[email protected]>

codesome · 2019-01-16T15:55:36Z

Was not able to address this yet: #374 (comment), rest all reviews have been addressed.

Signed-off-by: Ganesh Vernekar <[email protected]>

Signed-off-by: Krasi Georgiev <[email protected]>

Pull/374 review

Signed-off-by: Ganesh Vernekar <[email protected]>

codesome · 2019-01-17T14:33:55Z

ping @krasi-georgiev @gouthamve

krasi-georgiev · 2019-01-17T15:38:28Z

LGTM

gouthamve

I see that this could break compactions in subtle ways if the wrong ranges are compacted together. Should return only one block for rewriting tombstones.

compact.go

Signed-off-by: Krasi Georgiev <[email protected]>

krasi-georgiev · 2019-01-18T08:21:36Z

all done

* Dont write empty blocks when a compaction results in a block with no samples. Signed-off-by: Ganesh Vernekar <[email protected]>

https://github.com/prometheus/prometheus/releases Some of these changes seem to be interesting enough to update [ENHANCEMENT] Query performance improvements. prometheus-junkyard/tsdb#531 [BUGFIX] Scrape: catch errors when creating HTTP clients #5182. Adds new metrics: prometheus_target_scrape_pools_* deprecating the flag storage.tsdb.retention -> use storage.tsdb.retention.time [FEATURE] Add subqueries to PromQL. [ENHANCEMENT] Kubernetes SD: Add service external IP and external name to the discovery metadata. #4940 [ENHANCEMENT] Add metric for number of rule groups loaded. #5090 BUGFIX] Make sure the retention period does not overflow. #5112 [BUGFIX] Make sure the blocks do not get very large. #5112 [BUGFIX] Do not generate blocks with no samples. prometheus-junkyard/tsdb#374 [BUGFIX] Reintroduce metric for WAL corruptions. prometheus-junkyard/tsdb#473 Signed-off-by: Mikkel Oscar Lyderik Larsen <[email protected]>

krasi-georgiev reviewed Sep 10, 2018

View reviewed changes

compact.go Outdated Show resolved Hide resolved

Dont write empty blocks

beb0d73

Signed-off-by: Ganesh Vernekar <[email protected]>

codesome force-pushed the dont-write-empty-blocks branch from d2d6cd1 to beb0d73 Compare September 10, 2018 10:28

Added unit test TestNoEmptyBlock

bb76c82

Signed-off-by: Ganesh Vernekar <[email protected]>

krasi-georgiev mentioned this pull request Sep 17, 2018

Prevent creation of block dirs with zero samples #245

Closed

codesome mentioned this pull request Sep 18, 2018

Incorrect NumTombstones after block.Delete(...) #384

Closed

Fix infinite loop while compacting head.

c4edbcc

When number of blocks becomes 0, head is not truncated in reload(). This fixes it. Signed-off-by: Ganesh Vernekar <[email protected]>

codesome commented Sep 18, 2018

View reviewed changes

db_test.go Outdated Show resolved Hide resolved

krasi-georgiev reviewed Sep 19, 2018

View reviewed changes

compact.go Show resolved Hide resolved

db.go Show resolved Hide resolved

db_test.go Outdated Show resolved Hide resolved

db_test.go Outdated Show resolved Hide resolved

db_test.go Outdated Show resolved Hide resolved

db_test.go Outdated Show resolved Hide resolved

krasi-georgiev reviewed Sep 19, 2018

View reviewed changes

db.go Show resolved Hide resolved

db_test.go Outdated Show resolved Hide resolved

db_test.go Outdated Show resolved Hide resolved

krasi-georgiev reviewed Sep 21, 2018

View reviewed changes

db.go Outdated Show resolved Hide resolved

block.go Outdated Show resolved Hide resolved

Fix review comments

ab14c9c

Signed-off-by: Ganesh Vernekar <[email protected]>

codesome force-pushed the dont-write-empty-blocks branch from 98b38fd to 7623f3e Compare September 21, 2018 07:51

Krasi Georgiev added 2 commits January 16, 2019 14:49

Merge remote-tracking branch 'upstream/master' into pull/374-review

98988fd

Signed-off-by: Krasi Georgiev <[email protected]>

rebased

e0bb757

Signed-off-by: Krasi Georgiev <[email protected]>

krasi-georgiev reviewed Jan 16, 2019

View reviewed changes

db_test.go Outdated Show resolved Hide resolved

db_test.go Outdated Show resolved Hide resolved

db_test.go Outdated Show resolved Hide resolved

db_test.go Outdated Show resolved Hide resolved

db_test.go Outdated Show resolved Hide resolved

codesome added 2 commits January 16, 2019 20:36

Merge pull request #4 from krasi-georgiev/pull/374-review

efa23ce

Pull/374 review

Revert returning after empty block from head

0eed036

Signed-off-by: Ganesh Vernekar <[email protected]>

krasi-georgiev mentioned this pull request Jan 16, 2019

Added storage size based retention method and new metrics #343

Merged

Fix review comments

271b98f

Signed-off-by: Ganesh Vernekar <[email protected]>

codesome and others added 9 commits January 16, 2019 21:42

Dont create empty blocks in tests

a6a1779

Signed-off-by: Ganesh Vernekar <[email protected]>

nits

2b509e7

Signed-off-by: Ganesh Vernekar <[email protected]>

simplified test

ad9e3ed

Signed-off-by: Krasi Georgiev <[email protected]>

Merge remote-tracking branch 'upstream/master' into pull/374-review

441ea45

Signed-off-by: Krasi Georgiev <[email protected]>

solved the mistery for double compaction

02dc732

Signed-off-by: Krasi Georgiev <[email protected]>

nit

162fa29

Signed-off-by: Krasi Georgiev <[email protected]>

less samples

9f24cde

Signed-off-by: Krasi Georgiev <[email protected]>

Merge pull request #5 from krasi-georgiev/pull/374-review

2cb745f

Pull/374 review

Nits

45bde4c

Signed-off-by: Ganesh Vernekar <[email protected]>

gouthamve suggested changes Jan 17, 2019

View reviewed changes

compact.go Outdated Show resolved Hide resolved

compact.go Outdated Show resolved Hide resolved

compact.go Outdated Show resolved Hide resolved

revert some changes

6b87a56

Signed-off-by: Krasi Georgiev <[email protected]>

krasi-georgiev merged commit 1a9d08a into prometheus-junkyard:master Jan 18, 2019

codesome deleted the dont-write-empty-blocks branch January 18, 2019 11:03

radeklesniewski pushed a commit to SabreOSS/tsdb that referenced this pull request Jan 18, 2019

Don't write empty blocks (prometheus-junkyard#374)

3ed9615

* Dont write empty blocks when a compaction results in a block with no samples. Signed-off-by: Ganesh Vernekar <[email protected]>

mikkeloscar mentioned this pull request Apr 7, 2019

Update prometheus to v2.8.1 zalando-incubator/kubernetes-on-aws#1979

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't write empty blocks #374

Don't write empty blocks #374

codesome commented Sep 8, 2018 •

edited

Loading

krasi-georgiev commented Sep 11, 2018

krasi-georgiev commented Sep 11, 2018

gouthamve commented Sep 11, 2018

codesome commented Sep 11, 2018

krasi-georgiev commented Sep 11, 2018

codesome commented Sep 12, 2018

krasi-georgiev commented Sep 12, 2018

gouthamve commented Sep 12, 2018

krasi-georgiev commented Sep 17, 2018

codesome commented Sep 17, 2018

krasi-georgiev commented Sep 17, 2018

codesome commented Sep 18, 2018 •

edited

Loading

krasi-georgiev commented Sep 18, 2018

codesome commented Sep 18, 2018

codesome commented Sep 19, 2018

krasi-georgiev commented Sep 19, 2018

codesome commented Jan 16, 2019

codesome commented Jan 17, 2019

krasi-georgiev commented Jan 17, 2019

gouthamve left a comment

krasi-georgiev commented Jan 18, 2019

Don't write empty blocks #374

Don't write empty blocks #374

Conversation

codesome commented Sep 8, 2018 • edited Loading

krasi-georgiev commented Sep 11, 2018

krasi-georgiev commented Sep 11, 2018

gouthamve commented Sep 11, 2018

codesome commented Sep 11, 2018

krasi-georgiev commented Sep 11, 2018

codesome commented Sep 12, 2018

krasi-georgiev commented Sep 12, 2018

gouthamve commented Sep 12, 2018

krasi-georgiev commented Sep 17, 2018

codesome commented Sep 17, 2018

krasi-georgiev commented Sep 17, 2018

codesome commented Sep 18, 2018 • edited Loading

krasi-georgiev commented Sep 18, 2018

codesome commented Sep 18, 2018

codesome commented Sep 19, 2018

krasi-georgiev commented Sep 19, 2018

codesome commented Jan 16, 2019

codesome commented Jan 17, 2019

krasi-georgiev commented Jan 17, 2019

gouthamve left a comment

Choose a reason for hiding this comment

krasi-georgiev commented Jan 18, 2019

codesome commented Sep 8, 2018 •

edited

Loading

codesome commented Sep 18, 2018 •

edited

Loading