refactor block size calculation #637

krasi-georgiev · 2019-06-21T12:06:20Z

It now includes the size of the meta file itself as well for a more
correct block size.
It fixes a bug where the size didn't change when calling block.Delete.
Adds a dedicated test to ensure correct block sizes.
To allow opening the db in a read only mode it ensures the meta file is
not rewritten when the block size hasn't change .

Signed-off-by: Krasi Georgiev 8903888+krasi-georgiev@users.noreply.github.com

It now includes the size of the meta file itself as well for a more correct block size. It fixes a bug where the size didn't change when calling block.Delete. Adds a dedicated test to ensure correct block sizes. To allow opening the db in a read only mode it ensures the meta file is not rewritten when the block size hasn't change . Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

codesome

Just some nits, looks good otherwise.

tombstones.go

block.go

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

bwplotka · 2019-06-24T10:13:47Z

Taking look now (:

bwplotka

How it looks from compatibility standpoint? We are changing meta.json so old meta.json NumBytes will be ignored. Is that ok?

krasi-georgiev · 2019-06-24T10:17:29Z

I don't see any problem as when opening the db it will re-write the meta to the new version because of this check
https://github.com/prometheus/tsdb/pull/637/files#diff-4c74d030dafa9036e73f63926059db29R363

krasi-georgiev · 2019-06-24T10:26:12Z

just tested it and works as expected, all meta files are rewritten to included the correct block size information for each piece.

bwplotka

Ok, so I checked and spoke offline with @krasi-georgiev and it seems like NumBytes was added for size retention and that was the main use case. I like that we count it more precisely now.

However I find it quite odd that it's not populated on compaction, only on OpenBlock and rewritten back in meta.json. Especially for read only mode, we now base on fact that OpenBlock has to be opened by write mode TSDB first before we access the meta.json etc.. That's quite weird. Plus making sure compatibilty for meta.json is kept (what if some other system already base on NumBytes)?

So my question is: Do we really need to bake num of bytes in meta.json even though we calculate precise number in OpenBlock anyway? (even just to check if we have to rewritte)!

Also if we would stop writing this to meta.json we can totally remove this interesting calculation on how many bytes meta.json will have IF we would write number of those bytes etc.. it's quite complex (:

What do you think guys?

block.go

brian-brazil · 2019-06-24T11:03:11Z

By chance today's blog post uses numBytes. Populating it correctly on compaction seems like it would make more sense.

codesome · 2019-06-24T11:05:50Z

So my question is: Do we really need to bake num of bytes in meta.json even though we calculate precise number in OpenBlock anyway? (even just to check if we have to rewritte)!

This makes me think that we don't need any size info in the meta at all for size based retention.

bwplotka · 2019-06-24T11:22:37Z

This makes me think that we don't need any size info in the meta at all for size based retention.

Indeed. However if we already have docs for it I wonder what's the use case we described for NumBytes there. Can you link the post @brian-brazil here?

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev · 2019-06-24T11:34:24Z

How it looks from compatibility standpoint? We are changing meta.json so old meta.json NumBytes will be ignored. Is that ok?

I added a test to ensure that blocks with missing size info get recalculated and the meta file is rewritten at open.

By chance today's blog post uses numBytes. Populating it correctly on compaction seems like it would make more sense.

I already considered this option(writing the meta size info at compaction when creating a block), but there would be blocks that would never be touched by compaction(e.g. blocks that are too big already).

This makes me think that we don't need any size info in the meta at all for size based retention.

I considered this option as well, but using the meta simplifies the implementation quite a bit.

btw I think everyone should focus on block.Size() rather than the actual meta format. We can probably add some metric so that Brian's blog post uses that instead of relying on the meta format.

bwplotka · 2019-06-24T11:46:12Z

Yes but the meta json format takes the most lines of this PR and the most questions.

How keeping this size in meta helps in implementation? we do .Size of everything on every open block anyway in this PR (:

Looking on article: https://www.robustperception.io/how-much-disk-space-do-prometheus-blocks-use

It seems like use case is to tell block sizes. I would say log line with calculated block size on compaction would be enough for this use case. Or metric as Krasi suggest.

I would vote for removing any num size from stats and just calculating on open block.

krasi-georgiev · 2019-06-24T11:57:56Z

I would vote for removing any num size from stars and just calculating on open block.

That would probably work as well, but I thought that having it in the actual meta would simplify debugging or some other use cases where people would want to know the block size just by looking at the block meta.

codesome · 2019-06-24T12:02:20Z

having it in the actual meta would simplify debugging or some other use cases where people would want to know the block size just by looking at the block meta.

I thought of this when I reviewed, but then a simple du command would also be enough.

krasi-georgiev · 2019-06-24T12:03:32Z

I thought of this, but then a simple du command would also be enough.

yep, ok I will refactor to NOT use the meta file and lets see how it looks than.

bwplotka · 2019-06-24T12:03:42Z

Agree with @codesome - it's really easy to see this using linux tooling / object storage meta or whatever else. If any, we can discuss it in another PRs, right? (:

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev · 2019-06-24T13:12:25Z

new version ready.

bwplotka

Some minor suggestions, but LGTM otherwise. Thanks!

bwplotka · 2019-06-24T13:21:40Z

CHANGELOG.md

@@ -1,5 +1,7 @@
 ## master / unreleased
 - [FEATURE] Provide option to compress WAL records using Snappy. [#609](https://github.com/prometheus/tsdb/pull/609)
+ - [BUGFIX] Re-calculate block size when calling `block.Delete`.
+ - [CHANGE] `BlockStats` no longer holds size information and this is kept in memory.


Suggested change

- [CHANGE] `BlockStats` no longer holds size information and this is kept in memory.

- [CHANGE] `BlockStats` no longer holds (not precise) size information. This is now dynamically calculate while opening a block in memory.

the size is recalculated NOT just when opening a block, but also when calling block.Delete so I prefer to keep this a bit more generic.

Fair. Can we have then first fix (not precise) and then This is now dynamically calculated and kept in memory.

what do you mean by `(not precise)'? If that is the missing meta size that it can be something like:

- [CHANGE] `BlockStats` no longer holds size information. This is now dynamically calculated and kept in memory. It also includes the meta file size which was not included before.

bwplotka · 2019-06-24T13:23:10Z

block.go

@@ -285,6 +284,11 @@ type Block struct {
 	tombstones TombstoneReader

 	logger log.Logger
+
+	numBytesChunks    int64


uint64 and separate struct maybe for those? Not a blocker.

all Size,Write interfaces I have seen so far use int or int64 so I prefer to keep it as is.
Also for db.opts.MaxBytes we also use int64 where -1 means that the size retention is disabled.

Ok, fine by me then

Thanks for explaining

bwplotka · 2019-06-24T13:29:53Z

block.go

@@ -437,7 +428,9 @@ func (pb *Block) GetSymbolTableSize() uint64 {

 func (pb *Block) setCompactionFailed() error {
 	pb.meta.Compaction.Failed = true
-	return writeMetaFile(pb.logger, pb.dir, &pb.meta)
+	n, err := writeMetaFile(pb.logger, pb.dir, &pb.meta)
+	pb.numBytesMeta = n


Slight inconsistency with below code. We update size only on no error, here we do it no matter what. Can we do it on no error here as well? (: this will avoid potential confusion.

bwplotka · 2019-06-24T13:31:14Z

compact.go

 					level.Error(c.logger).Log(
 						"msg", "Failed to write 'Deletable' to meta file after compaction",
 						"ulid", b.meta.ULID,
 					)
 				}
+				b.numBytesMeta = n


Why we set numBytes to new block, but we ignore the other sizes later on?

actually this is setting the numBytesMeta for the existing blocks. Existing blocks are already open so need to set the size information when it changes.

Later on it is creating new blocks and these would have their size calculated when they are open at db.reload().

Good point, LGTM.

block.go

bwplotka

LGTM, super minor nit only (:

bwplotka · 2019-06-24T15:03:58Z

block.go

-	return writeMetaFile(pb.logger, pb.dir, &pb.meta)
+	n, err := writeMetaFile(pb.logger, pb.dir, &pb.meta)
+	if err != nil {
+		pb.numBytesMeta = 0


Not really -> I would just not set anything. (:

bwplotka · 2019-06-24T15:04:22Z

compact.go

 					level.Error(c.logger).Log(
 						"msg", "Failed to write 'Deletable' to meta file after compaction",
 						"ulid", b.meta.ULID,
 					)
 				}
+				b.numBytesMeta = n


Good point, LGTM.

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev · 2019-06-24T15:29:16Z

@brian-brazil This one is needed to unblock the db read only PR so will merge with the current implementation, but feel free to try and refactor at some point if you have a different idea.
Once we have the read only db working and the tests for it it should be easier to refactor and make sure it doesn't brake the read only db mode.

krasi-georgiev force-pushed the calculate-block-size-refactor branch 2 times, most recently from 9022985 to 26a8eca Compare June 21, 2019 12:17

krasi-georgiev force-pushed the calculate-block-size-refactor branch from 26a8eca to 64101d9 Compare June 21, 2019 12:19

codesome reviewed Jun 24, 2019

View reviewed changes

tombstones.go Outdated Show resolved Hide resolved

block.go Show resolved Hide resolved

block.go Show resolved Hide resolved

block.go Outdated Show resolved Hide resolved

block.go Outdated Show resolved Hide resolved

nits

321002e

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev force-pushed the calculate-block-size-refactor branch from d78542a to 321002e Compare June 24, 2019 10:00

changelog entry

9fcf803

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

codesome approved these changes Jun 24, 2019

View reviewed changes

bwplotka reviewed Jun 24, 2019

View reviewed changes

bwplotka suggested changes Jun 24, 2019

View reviewed changes

block.go Outdated Show resolved Hide resolved

block.go Outdated Show resolved Hide resolved

block.go Outdated Show resolved Hide resolved

block.go Outdated Show resolved Hide resolved

block.go Outdated Show resolved Hide resolved

test to ensure size recalculating when size data is missing.

73547a6

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev added 3 commits June 24, 2019 16:01

keep the block size in memory and not in the metafile

b9e94c5

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

Merge branch 'master' into calculate-block-size-refactor

f83fc26

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

nit

af26017

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

bwplotka reviewed Jun 24, 2019

View reviewed changes

bwplotka approved these changes Jun 24, 2019

View reviewed changes

krasi-georgiev added 2 commits June 24, 2019 18:21

nits

deb9b07

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

changelog

c7e140f

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>

krasi-georgiev force-pushed the calculate-block-size-refactor branch from 9e0d32c to c7e140f Compare June 24, 2019 15:23

krasi-georgiev merged commit 8d86e92 into prometheus-junkyard:master Jun 24, 2019

krasi-georgiev deleted the calculate-block-size-refactor branch June 24, 2019 15:42

bwplotka mentioned this pull request Jun 24, 2019

Make SafeDelete faster for large blocks; TSDB block repair case thanos-io/thanos#1056

Merged

codesome mentioned this pull request Jul 3, 2019

Cut v2.11.0-rc.0 prometheus/prometheus#5729

Merged

	- [CHANGE] `BlockStats` no longer holds size information and this is kept in memory.
	- [CHANGE] `BlockStats` no longer holds (not precise) size information. This is now dynamically calculate while opening a block in memory.

refactor block size calculation #637

refactor block size calculation #637

Conversation

krasi-georgiev commented Jun 21, 2019 • edited Loading

codesome left a comment

Choose a reason for hiding this comment

bwplotka commented Jun 24, 2019

bwplotka left a comment

Choose a reason for hiding this comment

krasi-georgiev commented Jun 24, 2019 • edited Loading

krasi-georgiev commented Jun 24, 2019

bwplotka left a comment • edited Loading

Choose a reason for hiding this comment

brian-brazil commented Jun 24, 2019

codesome commented Jun 24, 2019

bwplotka commented Jun 24, 2019

krasi-georgiev commented Jun 24, 2019 • edited Loading

bwplotka commented Jun 24, 2019 • edited Loading

krasi-georgiev commented Jun 24, 2019

codesome commented Jun 24, 2019 • edited Loading

krasi-georgiev commented Jun 24, 2019

bwplotka commented Jun 24, 2019

krasi-georgiev commented Jun 24, 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

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

krasi-georgiev Jun 24, 2019 • edited Loading

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

Choose a reason for hiding this comment

krasi-georgiev commented Jun 24, 2019 • edited Loading

krasi-georgiev commented Jun 21, 2019 •

edited

Loading

krasi-georgiev commented Jun 24, 2019 •

edited

Loading

bwplotka left a comment •

edited

Loading

krasi-georgiev commented Jun 24, 2019 •

edited

Loading

bwplotka commented Jun 24, 2019 •

edited

Loading

codesome commented Jun 24, 2019 •

edited

Loading

krasi-georgiev Jun 24, 2019 •

edited

Loading

krasi-georgiev commented Jun 24, 2019 •

edited

Loading