[breaking/format] Add key-offset index to the end of SST table #881

jarifibrahim · 2019-06-20T11:26:12Z

The original PR #816 got closed because I deleted the release/v2.1.0 branch (we didn't need it) and Github won't let me reopen that PR or change the target branch.

This is a breaking change. Older versions of badger data will not work after this change

This commit adds the key-offset index used for searching the blocks in a table to the end of the SST file. The length of the index is stored in the last 4 bytes of the file.

Earlier we used to build the index at runtime by reading the keys stored at the offset specified in the footer of the file. With this change, we won't build the index, we'll read it from the file.

Benchmarks

The following test was run on a badger DB with 300 million sorted key value pairs (DB size 57GB)

With new footer (the proposed changes in this PR)

go test -run=^$ github.com/dgraph-io/badger -bench ^BenchmarkDBOpen$ -benchdir="/home/ibrahim/partition/300-million-sorted-footer" -v
badger 2019/05/28 21:31:20 INFO: 92 tables out of 155 opened in 3.005s
badger 2019/05/28 21:31:22 INFO: All 155 tables opened in 5.132s
badger 2019/05/28 21:31:22 INFO: Replaying file id: 299 at offset: 116366000
badger 2019/05/28 21:31:22 INFO: Replay took: 355.887µs
goos: linux
goarch: amd64
pkg: github.com/dgraph-io/badger
BenchmarkDBOpen-8   	       1	5228268412 ns/op
PASS
ok  	github.com/dgraph-io/badger	5.291s

With existing implementation

go test -run=^$ github.com/dgraph-io/badger -bench ^BenchmarkDBOpen$ -benchdir="/home/ibrahim/partition/300-million-sorted" -v
badger 2019/05/28 21:41:46 INFO: 17 tables out of 155 opened in 3.337s
badger 2019/05/28 21:41:49 INFO: 34 tables out of 155 opened in 6.099s
badger 2019/05/28 21:41:52 INFO: 55 tables out of 155 opened in 9.267s
badger 2019/05/28 21:41:55 INFO: 72 tables out of 155 opened in 12.164s
badger 2019/05/28 21:41:58 INFO: 91 tables out of 155 opened in 15.165s
badger 2019/05/28 21:42:01 INFO: 109 tables out of 155 opened in 18.159s
badger 2019/05/28 21:42:04 INFO: 126 tables out of 155 opened in 21.105s
badger 2019/05/28 21:42:07 INFO: 145 tables out of 155 opened in 24.072s
badger 2019/05/28 21:42:09 INFO: All 155 tables opened in 25.695s
badger 2019/05/28 21:42:09 INFO: Replaying file id: 299 at offset: 116366000
badger 2019/05/28 21:42:09 INFO: Replay took: 100.651µs
goos: linux
goarch: amd64
pkg: github.com/dgraph-io/badger
BenchmarkDBOpen-8   	       1	25776272170 ns/op
PASS
ok  	github.com/dgraph-io/badger	25.858s

The following code was used for benchmarking (also added to db2_test.go file)

// The following benchmark test is supposed to be run against a badger directory with some data.
// Use badger fill to create data if it doesn't exist.
func BenchmarkDBOpen(b *testing.B) {
	if *benchDir == "" {
		b.Skip("Please set -benchdir to badger directory")
	}
	dir := *benchDir
	opt := DefaultOptions
	opt.Dir = dir
	opt.ValueDir = dir
	opt.ReadOnly = true
	for i := 0; i < b.N; i++ {
		db, err := Open(opt)
		require.NoError(b, err)
		require.NoError(b, db.Close())
	}
}

This change is

This commit adds the key-offset index used for searching the blocks in a table to the end the SST file. The length of the index is stored in the last 4 bytes of the file.

…o/badger into ibrahim/footer-protobuf

martinmr

Reviewed 14 of 14 files at r1.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @ashish-goswami, @jarifibrahim, and @manishrjain)

db2_test.go, line 105 at r1 (raw file):

var manual = flag.Bool("manual", false, "Set when manually running some tests.")

// Badger dir to be used for performing db.Open benchmark

minor: period at end of comments.

table/builder.go, line 160 at r1 (raw file):

	b.addHelper([]byte{}, y.ValueStruct{})
	y.AssertTrue(b.buf.Len() < math.MaxUint32)
	// Add key to the block index

minor: period at the end.

table/builder.go, line 233 at r1 (raw file):

	}

	// Write bloom filter.

why is the bloom filter being written before the block index now?

table/builder.go, line 242 at r1 (raw file):

	y.AssertTrue(n < math.MaxUint32)
	// Write index size

nit: period at the end.

table/builder.go, line 253 at r1 (raw file):

func (b *Builder) writeChecksum(data []byte) {
	// Build checksum for the index

nit: period at the end

table/builder.go, line 267 at r1 (raw file):

	}

	// Write checksum to the file

minor: period at the end.

table/builder.go, line 274 at r1 (raw file):

	y.AssertTrue(n < math.MaxUint32)
	// Write checksum size

minor: period at the end.

table/builder_test.go, line 25 at r1 (raw file):

	"github.com/dgraph-io/badger/options"
	"github.com/dgraph-io/badger/y"

@campoy is making a change to move to v2: #882

If that is submitted before your change, merge and make sure you update the references. So your imports here would become:

	"github.com/dgraph-io/badger/v2/options"
	"github.com/dgraph-io/badger/v2/y"

Same for other badger imports you added.

table/builder_test.go, line 36 at r1 (raw file):

		require.Len(t, table.blockIndex, 1)
	})
	t.Run("multiple keys", func(t *testing.T) {

If these tests are independent, just have them in separate methods.

table/table.go, line 223 at r1 (raw file):

	readPos := t.tableSize

	// Read checksum len from the last 4 bytes

minor: period at the end

table/table.go, line 228 at r1 (raw file):

	checksumLen := binary.BigEndian.Uint32(buf)

	// Read checksum

minor: period at the end

table/table.go, line 236 at r1 (raw file):

	}

	// Read index size from the footer

minor: period at the end

table/table.go, line 240 at r1 (raw file):

	buf = t.readNoFail(readPos, 4)
	indexLen := int(binary.BigEndian.Uint32(buf))
	// Read index

minor: period at the end

table/table.go, line 265 at r1 (raw file):

	ko := t.blockIndex[idx]
	blk := block{
		offset: int(ko.Offset),

I see a lot of conversion between int types in this file. Is there a way to avoid that?

jarifibrahim

Reviewable status: 3 of 14 files reviewed, 4 unresolved discussions (waiting on @ashish-goswami, @campoy, @manishrjain, and @martinmr)

table/builder.go, line 233 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

why is the bloom filter being written before the block index now?

We're writing bloom filter along with the index. They're part of the same protobuf.
I'll reword the comment so that it makes more sense.

table/builder_test.go, line 25 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

@campoy is making a change to move to v2: #882

If that is submitted before your change, merge and make sure you update the references. So your imports here would become:
	"github.com/dgraph-io/badger/v2/options"
	"github.com/dgraph-io/badger/v2/y"
Same for other badger imports you added.

Yes. I've changed it. Thanks!

table/builder_test.go, line 36 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

If these tests are independent, just have them in separate methods.

Both the tests are related to validation of the table index and this looks like a logical grouping to me.
Keeping it this way makes it more readable as well.

table/table.go, line 265 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

I see a lot of conversion between int types in this file. Is there a way to avoid that?

Not really. t.read(..) needs ints and protobuf has int16, int32, int64. No matter which type we use, we'll have to convert it to int.

One thing we can do is change the param type of t.read(..) from int to int64. The function internally converts int to int64 and then we can use int64 everywhere.

manishrjain

Got a couple of comments.

Reviewed 5 of 11 files at r2.
Reviewable status: 8 of 14 files reviewed, 5 unresolved discussions (waiting on @ashish-goswami, @campoy, @jarifibrahim, and @martinmr)

table/builder.go, line 191 at r1 (raw file):

// ReachedCapacity returns true if we... roughly (?) reached capacity?
func (b *Builder) ReachedCapacity(cap int64) bool {
	estimateSz := b.buf.Len() + 8 /* empty header */ + 4 /* Index length */ +

Move each + to a new line and use // instead of /*.

table/table.go, line 265 at r1 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

Not really. t.read(..) needs ints and protobuf has int16, int32, int64. No matter which type we use, we'll have to convert it to int.

One thing we can do is change the param type of t.read(..) from int to int64. The function internally converts int to int64 and then we can use int64 everywhere.

That sounds like a good idea.

…o/badger into ibrahim/footer-protobuf

jarifibrahim

Reviewable status: 7 of 14 files reviewed, 5 unresolved discussions (waiting on @ashish-goswami, @campoy, @manishrjain, and @martinmr)

table/builder.go, line 191 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Move each + to a new line and use // instead of /*.

Done.

table/table.go, line 265 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

That sounds like a good idea.

I'll defer the variable type change to a separate PR. The int => int64 change requires changes at multiple places in the code.

If we do not stop the existing one, the go routines will keep running in the background.

When the opened DB is not closed, all the running go routines (watermark, compaction, etc) are not stopped and they keep running in the background

jarifibrahim · 2019-06-24T13:08:03Z

I had to push ec7de22 and 127ff52 to reduce error logs in the build output. Without those commits, the PR build was failing and the errors were not visible because Travis has a limit on the number of characters in the logs. See https://travis-ci.org/dgraph-io/badger/jobs/549661427 for an example of failed build.

martinmr

Reviewed 6 of 11 files at r2, 3 of 3 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ashish-goswami and @manishrjain)

This reverts commit 96d00d8.

Ibrahim Jarif added 30 commits May 20, 2019 21:57

Add key-offset index to the end of SST table

e711436

This commit adds the key-offset index used for searching the blocks in a table to the end the SST file. The length of the index is stored in the last 4 bytes of the file.

Add checksum for the footer

50bcc19

Fix comment

fe25fce

Fix comment

1fe7c8e

address review comments

fc950be

Fix manifest test

57cdc34

fix table tests

b08af1f

Return from compactBuildTables on failure

12fc3f5

Improved error handling

372dec4

fixup

6267723

error checking for verifychecksum

19f3b51

Use y.check instead of err != nil for buf.write

d64dc00

blockIndex does not return error

9dd9874

Use CRC32 instead of sha256

fb1ed14

Wrap line at 100

66053f2

remove debugging code

7ea15d2

Ensure we do not overflow uint32

fde3f5a

Refactor compactBuildTables function

bc5ca72

Add link to benchmark in code

c816599

fixup

87acb80

code cleanup and remove unreleated changes

17f6109

Merge branch 'master' into ibrahim/footer-protobuf

4369afe

fixup

413ce9e

Address review comments

aca502d

Remove checksum from manifest file

362779c

Add benchmark test

62a2abf

Change pb.checksum format and add checksum.go file

db8bb57

add missing licence in checksum.go

4ee98ce

Address golangci

465dc10

Merge branch 'master' into ibrahim/footer-protobuf

6793ba7

Ibrahim Jarif added 3 commits June 4, 2019 18:38

Merge branch 'ibrahim/footer-protobuf' of https://github.com/dgraph-i…

76917cb

…o/badger into ibrahim/footer-protobuf

Merge branch 'master' into ibrahim/footer-protobuf

21874ff

Add table structure in code comments

0109f62

jarifibrahim requested review from ashish-goswami and manishrjain as code owners June 20, 2019 11:26

jarifibrahim requested a review from a team June 20, 2019 11:26

jarifibrahim mentioned this pull request Jun 20, 2019

[breaking/format] Add key-offset index to the end of SST table #816

Closed

jarifibrahim added this to the v2.1.0 milestone Jun 20, 2019

Merge branch 'master' into ibrahim/footer-protobuf

41cbfbb

martinmr assigned jarifibrahim Jun 20, 2019

martinmr suggested changes Jun 20, 2019

View reviewed changes

Ibrahim Jarif added 3 commits June 21, 2019 15:27

Address review comments

67694c7

Merge branch 'master' into ibrahim/footer-protobuf

cc35f78

Fix v2 import paths

96d00d8

jarifibrahim commented Jun 21, 2019

View reviewed changes

manishrjain approved these changes Jun 21, 2019

View reviewed changes

Ibrahim Jarif added 3 commits June 24, 2019 15:23

move each comment on a new line

e552863

Merge branch 'ibrahim/footer-protobuf' of https://github.com/dgraph-i…

72b45d8

…o/badger into ibrahim/footer-protobuf

Fixup

4dd42c9

jarifibrahim commented Jun 24, 2019

View reviewed changes

Ibrahim Jarif added 2 commits June 24, 2019 17:50

Stream Writer: Stop oracle before starting new one

ec7de22

If we do not stop the existing one, the go routines will keep running in the background.

Close all opened DBs

127ff52

When the opened DB is not closed, all the running go routines (watermark, compaction, etc) are not stopped and they keep running in the background

martinmr approved these changes Jun 24, 2019

View reviewed changes

Ibrahim Jarif added 3 commits June 27, 2019 15:38

Merge branch 'master' into ibrahim/footer-protobuf

b5a6568

Revert "Fix v2 import paths"

e486194

This reverts commit 96d00d8.

Use option builder in test

db16a22

jarifibrahim merged commit 61c492d into master Jul 5, 2019

jarifibrahim mentioned this pull request Jul 8, 2019

Improve Badger startup time #790

Closed

campoy deleted the ibrahim/footer-protobuf branch July 10, 2019 20:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[breaking/format] Add key-offset index to the end of SST table #881

[breaking/format] Add key-offset index to the end of SST table #881

Uh oh!

jarifibrahim commented Jun 20, 2019 •

edited by manishrjain

Loading

Uh oh!

martinmr left a comment

Uh oh!

jarifibrahim left a comment

Uh oh!

manishrjain left a comment

Uh oh!

jarifibrahim left a comment

Uh oh!

jarifibrahim commented Jun 24, 2019

Uh oh!

martinmr left a comment

Uh oh!

Uh oh!

[breaking/format] Add key-offset index to the end of SST table #881

[breaking/format] Add key-offset index to the end of SST table #881

Uh oh!

Conversation

jarifibrahim commented Jun 20, 2019 • edited by manishrjain Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

Uh oh!

martinmr left a comment

Choose a reason for hiding this comment

Uh oh!

jarifibrahim left a comment

Choose a reason for hiding this comment

Uh oh!

manishrjain left a comment

Choose a reason for hiding this comment

Uh oh!

jarifibrahim left a comment

Choose a reason for hiding this comment

Uh oh!

jarifibrahim commented Jun 24, 2019

Uh oh!

martinmr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jarifibrahim commented Jun 20, 2019 •

edited by manishrjain

Loading