[dbnode] Add index snapshotting by notbdu · Pull Request #2556 · m3db/m3

notbdu · 2020-08-26T04:45:57Z

What this PR does / why we need it:

Adds index snapshotting. This will reduce the amount of bootstrapping time since we won't need to build the entire index segment.

Also implements initial work for eagerly offloading terminal or frozen index segments from memory.

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

NONE

Does this PR require updating code package or user-facing documentation?:

NONE

ryanhall07

can you elaborate more on "why we need this" in the PR description?

ryanhall07 · 2020-09-17T16:43:56Z

 		}
+
+		// Read index snapshot files
+		indexSnapshotFiles, err := s.indexSnapshotFilesFn(


are we going to be reading snapshot files twice now (like the commit log)?

Yea, unfortunately :/. I think I can piggy back off of nate's work to cache read info file results between bootstrap runs to reduce this work after it lands: https://github.com/m3db/m3/pull/2598/files

I think this is also true for calls to ReadIndexInfofiles, ya? Either way, probably worth doing in a follow up PR so as to not expand the size of this one.

ryanhall07 · 2020-09-17T16:47:13Z

+		// NB(bodu): We don't actually bootstrap snapshot data in the fs bootstrapper
+		// (we do this in the commitlog bootstrapper) but we do want to subtract the shard time
+		// ranges fulfilled by index snapshots.
+		r, err = s.bootstrapFromIndexSnapshots(


I don't follow why you need to read index snapshots in both bootstrapers

Spoke offline.

ryanhall07 · 2020-09-17T16:48:59Z

 	QueryStats() stats.QueryStats
 }
+
+// BlockStateSnapshot represents a snapshot of a 's block state at


don't follow doc

ryanhall07 · 2020-09-17T16:49:19Z

+	snapshot     BootstrappedBlockStateSnapshot
+}
+
+// NewBlockStateSnapshot constructs a new NewBlockStateSnapshot.


constructs a new BlockStateSnapshot

robskillington · 2020-10-06T19:29:19Z

Can we add method "SegmentState" to the interface "IndexSegmentFileSetWriter"? Would rather explicitly use the default value returned by the segment file set writer rather than use it by default if we can't upcast.

codecov · 2020-10-09T21:04:38Z

Codecov Report

❌ Patch coverage is 75.36424% with 186 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.1%. Comparing base (2ac4853) to head (ed594f5).
⚠️ Report is 1117 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #2556     +/-   ##
========================================
- Coverage    72.1%   72.1%   -0.1%     
========================================
  Files        1099    1099             
  Lines       99581   99901    +320     
========================================
+ Hits        71863   72079    +216     
- Misses      22807   22885     +78     
- Partials     4911    4937     +26

Flag	Coverage Δ
aggregator	`75.7% <ø> (-0.1%)`	⬇️
cluster	`84.9% <ø> (ø)`
collector	`84.3% <ø> (ø)`
dbnode	`79.3% <76.8%> (-0.1%)`	⬇️
m3em	`74.4% <ø> (ø)`
m3ninx	`73.1% <27.2%> (-0.1%)`	⬇️
metrics	`17.2% <ø> (ø)`
msg	`74.2% <ø> (+0.1%)`	⬆️
query	`69.0% <ø> (ø)`
x	`80.1% <ø> (-0.1%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ac4853...ed594f5. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

robskillington · 2020-10-16T16:29:16Z

Note: With new transparent indexing, LoadBlock should not transparently index when this lands. Also, would be good to have in documentation the snapshot metadata files and how they all relate to each other.

robskillington · 2020-10-16T16:30:56Z

+// Since we are doing this after a successful flush, there is a small window where we can crash and
+// run into the above case. However, we'd rather deal w/ a one time extra mem than long bootstrap times
+// if we write an empty snapshot to disk and the node crashes before a successful warm flush.
+func (m *flushManager) writeEmptyIndexSnapshots(


@notbdu as discussed, is it possible at all to avoid writing empty index snapshots now we are tracking everything with snapshot ID?

So the issue of potentially bootstrapping a "stale" snapshot if the node crashes between a successful warm flush and the next snapshot did not go away. In this case, since we've already warm flushed, the loaded snapshot data would never get evicted.

Although... after revisiting this logic it makes more sense to me to just check during index bootstrapping whether or not the index block is sealed and we've successfully warm flushed and ignore any warm snapshots for that block.

What do you think of this approach:

for blockStart, blockResults := range bootstrapResults { block, err := i.ensureBlockPresentWithRLock(blockStart.ToTime()) if err != nil { // should never happen multiErr = multiErr.Add(i.unableToAllocBlockInvariantError(err)) continue } // NB(bodu): For warm snapshots, we need to make sure that we haven't already successfully warm // flushed that block start. We can run into this case when the node crashes between a successful warm // flush and the next index snapshot. if _, ok := blockResults.GetBlock(idxpersist.SnapshotWarmIndexVolumeType); ok { if block.IsSealed() && i.hasIndexWarmFlushedToDisk(infoFiles, blockStart.ToTime()) { // If we have warm snapshots and the block has been warm flushed already, // we just discard the warm snapshot data. blockResults.DeleteBlock(idxpersist.SnapshotWarmIndexVolumeType) } }

nbroyles · 2020-10-17T21:13:24Z

 	Close   IndexCloser
 }

+// PreparedIndexSnapshotPersist is an object that wraps holds a persist function and a closer.


nit: comment is a duplicate of the one above. Probably worth pointing out the difference in one of the two

nbroyles · 2020-10-17T21:39:54Z

 			// We may have less we need to read
 			shardTimeRanges = shardTimeRanges.Copy()
 			shardTimeRanges.Subtract(r.fulfilled)
+			log.Println("subtract ->", r.fulfilled)


Looks like this might be debug logging you can remove?

nbroyles · 2020-10-17T21:49:51Z

+			return bootstrap.NamespaceResults{}, err
+		}
+		fsOpts := s.opts.CommitLogOptions().FilesystemOptions()
+		indexInfoFiles := s.readIndexInfoFilesFn(fsOpts.FilePathPrefix(), ns.Metadata.ID(),


nit: think you already have variables for fsOpts and filePathPrefix created outside of this for loop

nbroyles · 2020-10-17T21:50:20Z

+		fsOpts := s.opts.CommitLogOptions().FilesystemOptions()
+		indexInfoFiles := s.readIndexInfoFilesFn(fsOpts.FilePathPrefix(), ns.Metadata.ID(),
+			fsOpts.InfoReaderBufferSize(), persist.FileSetSnapshotType)
+		if err != nil {


Can remove this if block

nbroyles · 2020-10-17T21:54:16Z

 			shardTimeRanges := ns.DataRunOptions.ShardTimeRanges
 			dataResult = shardTimeRanges.ToUnfulfilledDataResult()
 		}
-		var indexResult result.IndexBootstrapResult


Now each bootstrap.NamespaceResult will have the same IndexResult from line 203. Is that ok?

Ah, good catch the index results should be per ns.

nbroyles · 2020-10-19T13:44:43Z


+const (
+	// Volume index defaults to -1 or unset.
+	defaultVolumeIndex = -1


I think we default to 0 for data files. Should we do the same here or is it somewhat apples and oranges?

Sorry, the naming here is confusing I changed it to the following:

// Volume index of -1 means unset. volumeIndexUnset = -1

The default is 0 for index data too.

…string as key in index results.

…e on snapshots case in addition to the split across snapshots/commitlogs case. Add comments.

notbdu requested a review from robskillington August 26, 2020 04:45

nbroyles reviewed Aug 31, 2020

View reviewed changes

Comment thread src/dbnode/storage/bootstrap/bootstrapper/commitlog/source.go Outdated

notbdu force-pushed the bdu/index-snapshots branch from 2eb19ed to 1425f4f Compare September 9, 2020 22:05

notbdu changed the title ~~(WIP) [dbnode] Add index snapshotting~~ [dbnode] Add index snapshotting Sep 15, 2020

ryanhall07 reviewed Sep 17, 2020

View reviewed changes

notbdu force-pushed the bdu/index-snapshots branch 3 times, most recently from f20392a to 110de93 Compare September 24, 2020 19:19

notbdu force-pushed the bdu/index-snapshots branch 3 times, most recently from 8b0ec9b to 41781b5 Compare October 5, 2020 22:36

robskillington reviewed Oct 6, 2020

View reviewed changes

notbdu force-pushed the bdu/index-snapshots branch 3 times, most recently from 4e3f709 to da3bf91 Compare October 9, 2020 20:48

notbdu force-pushed the bdu/index-snapshots branch from 18e3fb0 to 49a8ba1 Compare October 9, 2020 23:19

robskillington assigned robskillington, ryanhall07 and nbroyles Oct 14, 2020

notbdu force-pushed the bdu/index-snapshots branch from 5ac62a8 to d6a26ab Compare October 16, 2020 01:24

robskillington reviewed Oct 16, 2020

View reviewed changes

nbroyles reviewed Oct 17, 2020

View reviewed changes

Comment thread src/dbnode/storage/types.go

nbroyles reviewed Oct 17, 2020

View reviewed changes

nbroyles reviewed Oct 19, 2020

View reviewed changes

notbdu added 24 commits November 3, 2020 17:35

Fix rebase conflicts and tests.

6aa8098

Fix cleanup test.

eba2dc0

Fix integration tests.

5ccdf71

Update mocks.

171f6af

Address PR feedback.

9b8c4e3

Remove delete warm snapshot block for now.

b965b2a

Use an actual index instead of the ns hash.

a0b04b5

Address PR comments and move around docs.

f676a96

Fix unit tests.

ab5c7f0

Fix integration test fn call.

0bca1dc

Split invariant violation logic.

0afd3e2

Can have no snapshots for certain blocks.

e72d6cf

Wait for warm flush to succeed.

2b293cb

Move digest notes to docs folder.

c894b71

Only emit invariant violation if error.

1351650

Don't mmap index snapshots after persisting.

912e0ba

Fix method calls and tests.

657bd82

Fix flaky docker integration test reads.

7f9a395

Return errors when buffer size too small.

b4e912a

Remove write empty index snapshots logic, update docs, and use ns ID …

4b9029a

…string as key in index results.

Address PR comments.

14efab4

Fn name change.

f284bac

Address PR feedback.

60b4ef0

Update commitlog bootstrap integration test to cover the full relianc…

dd5edea

…e on snapshots case in addition to the split across snapshots/commitlogs case. Add comments.

notbdu force-pushed the bdu/index-snapshots branch from a80540e to dd5edea Compare November 3, 2020 22:35

Update license header.

ed594f5

robskillington force-pushed the master branch from e130b7e to 3cf877e Compare November 12, 2020 20:21

ryanhall07 removed their assignment Oct 17, 2023

ryanhall07 requested review from ryanhall07 and removed request for ryanhall07 October 17, 2023 21:27

Conversation

notbdu commented Aug 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ryanhall07 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

robskillington commented Oct 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nbroyles Oct 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

notbdu commented Aug 26, 2020 •

edited

Loading

codecov Bot commented Oct 9, 2020 •

edited

Loading

robskillington commented Oct 16, 2020 •

edited

Loading

nbroyles Oct 17, 2020 •

edited

Loading