Cold flushes #1624

justinjc · 2019-05-09T20:13:10Z

What this PR does / why we need it:

This PR introduces cold flush logic that merges out of order writes with data on disk.

Special notes for your reviewer:
Sorry, this PR is rather large. A good place to start would be the ColdFlush function in shard.go, where it identifies which blockStarts require cold flushes, then uses the merger to merge data together. As a natural follow up, look into merger.go to review actual merge logic, and fs_merge_with_mem.go to review the logic for going through data in memory.

Please note that this PR is just one part of implementing the ColdWrites feature. Landing of this PR does not enable the ability to do cold writes without other necessary changes, e.g. in the seek manager. As such, most of the code here is feature flagged off by default. This flag is at the namespace level (in the option ColdWritesEnabled). The call to ColdFlush in namespace.go checks for this option, and returns success right away if the flag is false, hence, any logic below that (shard/series/buffer/actual merging) will never get run.

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

src/dbnode/network/server/tchannelthrift/node/service.go

src/dbnode/retention/times.go

src/dbnode/storage/flush.go

src/dbnode/storage/namespace.go

src/dbnode/storage/shard.go

src/dbnode/storage/namespace.go

codecov · 2019-05-17T22:19:51Z

Codecov Report

Merging #1624 into master will increase coverage by 0.6%.
The diff coverage is 43.4%.

@@           Coverage Diff            @@
##           master   #1624     +/-   ##
========================================
+ Coverage    71.3%     72%   +0.6%     
========================================
  Files         962     964      +2     
  Lines       80908   80318    -590     
========================================
+ Hits        57747   57839     +92     
+ Misses      19393   18703    -690     
- Partials     3768    3776      +8

Flag	Coverage Δ
#aggregator	`82.3% <ø> (-0.1%)`	⬇️
#cluster	`85.7% <ø> (ø)`	⬆️
#collector	`63.9% <ø> (ø)`	⬆️
#dbnode	`79.6% <43.4%> (-0.5%)`	⬇️
#m3em	`73.2% <ø> (ø)`	⬆️
#m3ninx	`74.1% <ø> (ø)`	⬆️
#m3nsch	`51.1% <ø> (ø)`	⬆️
#metrics	`17.6% <ø> (ø)`	⬆️
#msg	`74.9% <ø> (ø)`	⬆️
#query	`67.3% <ø> (+3.5%)`	⬆️
#x	`86.4% <ø> (-0.1%)`	⬇️

Continue to review full report at Codecov.

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

src/dbnode/persist/fs/merger.go

src/dbnode/storage/shard.go

src/dbnode/storage/fs_merge_with_mem.go

src/dbnode/storage/series/buffer.go

src/dbnode/storage/shard.go

src/dbnode/persist/fs/merger.go

richardartoul · 2019-05-28T19:14:59Z

src/dbnode/persist/fs/merger.go

+		NamespaceMetadata: nsMd,
+		Shard:             shard,
+		BlockStart:        startTime,
+		DeleteIfExists:    false,


Comment here please. Also, how does this work right now? I thought the flushPreparer thing would complain if the file already existed and you set this to false.

Is it because you're mocking everything right now?

Yeah, everything is mocked out right now. After the seek manager the numbered filesets change lands, DeleteIfExists should be false, so probably keep this false for now?

src/dbnode/persist/fs/merger.go

src/dbnode/storage/flush.go

src/dbnode/storage/fs_merge_with_mem.go

src/dbnode/storage/series/buffer.go

src/dbnode/storage/series/series.go

src/dbnode/storage/shard.go

prateek · 2019-05-31T19:15:04Z

src/dbnode/storage/database.go

 		curSchema, err := schemaReg.GetLatestSchema(metadata.ID())
 		if curSchema != nil {
-			curSchemaId = curSchema.DeployId()
+			curSchemaID = curSchema.DeployId()


not from your changes but looks like the error on line 236 is unchecked. what's the deal there?

Hm, not immediately obvious to me what to do on error, opened #1694.

prateek · 2019-05-31T19:18:11Z

src/dbnode/persist/fs/types.go

+	) error
+}
+
+// NewMergerFn is the function to call to get a new Merger. Mostly used as a


hm if it's only used for testing, does it need to be exported?

That comment is confusing - I removed it. What I meant is that I needed the ability to pass in the merger constructor in the cold flush code in shard.go (a different package) so that it is testable.

src/dbnode/storage/series/buffer.go

src/dbnode/storage/series/types.go

prateek · 2019-05-31T21:31:34Z

src/dbnode/persist/fs/merger.go

+// data, nor does it clean up the original fileset.
+func NewMerger(
+	reader DataFileSetReader,
+	blockAllocSize int,


nit: instead of all the args, could you make an Options type

src/dbnode/persist/fs/merger.go

robskillington · 2019-06-02T20:44:04Z

src/dbnode/storage/flush.go

@@ -63,6 +64,7 @@ type flushManager struct {
 	// are used for emitting granular gauges.
 	state           flushManagerState
 	isFlushing      tally.Gauge
+	isColdFlushing  tally.Gauge


Super nit, I wonder if we should call these a "compaction" flush instead of cold flush? I'm not really too fussed, but interested to hear thoughts cc @justinjc @prateek @richardartoul

(Like still keep the hot/cold terminology but just call these flushes "compaction flushes" because then if repairs uses this code path, it's not just cold writes that are being persisted per-se).

Second thoughts, seems to hard to rename at this point, let's not bother.

I actually renamed this from compact to cold flush midway through 😅. Maybe we'll re-tackle this when the usage of cold flush differs too much later.

robskillington · 2019-06-02T20:47:18Z

src/dbnode/storage/flush.go

+		multiErr = multiErr.Add(err)
+	}
+
+	return multiErr.FinalError()


Are we sure that if there is an error here that the commit logs won't get deleted? I think the commit log cleanup logic only checks that there is a valid snapshot, not that also cold flushes contained by those commit logs were written to disk no?

cc @richardartoul

Ah right, that's true. We can either:

Return if there's an error cold flushing

Don't clean up commit logs unless a successful cold flush has happened too

Both run the risk of using up too much disk space if cold flushes fail, but (1) is easier to implement, so I'll do that.

robskillington · 2019-06-02T20:49:03Z

src/dbnode/storage/fs_merge_with_mem.go

+
+	nextVersion := m.retriever.RetrievableBlockColdVersion(startTime) + 1
+
+	tmpCtx := context.NewContext()


I think you should get one from a context pool here or else reuse one (since you are calling BlockingClose which makes them reuseable).

Makes sense, I'll have Read take a context and share one across iterations.

src/dbnode/persist/fs/merger.go

richardartoul · 2019-06-04T18:06:56Z

src/dbnode/persist/fs/merger.go

+		multiIter.ResetSliceOfSlices(xio.NewReaderSliceOfSlicesFromBlockReadersIterator(brs), nsCtx.Schema)
+
+		// tagsIter is never nil.
+		tags, err := convert.TagsFromTagsIter(id, tagsIter, identPool)


@justinjc Yes I think with the restructuring you've done this is now safe since the tags will be valid as long as the IDs are valid, and the IDs are valid for the duration of the file writing. Can you add a comment explaining this now?

src/dbnode/persist/fs/merger.go

src/dbnode/storage/fs_merge_with_mem.go

src/dbnode/storage/namespace.go

src/dbnode/storage/series/buffer.go

CLAassistant · 2019-06-05T03:27:21Z

All committers have signed the CLA.

src/dbnode/persist/fs/merger.go

src/dbnode/storage/shard.go

src/dbnode/storage/flush.go

src/dbnode/storage/shard.go

richardartoul · 2019-06-07T14:09:31Z

src/dbnode/persist/fs/merger.go

+
+// Merge merges data from a fileset with a merge target and persists it.
+// The caller is responsible for finalizing all resources used for the
+// MergeWith passed here.


Can you document somewhere (here or on ForEachRemaining) that while the data passed to ForEachRemaining is basically a copy that will be finalized when the context is closed, the ID and tags are expected to live for as long as the user of the MergeWith requires them so they should either be NoFinalize() or passed as copies (in your case since you're taking them from the shard/series they're NoFinalize() and its safe but would like to document)

richardartoul · 2019-06-07T14:11:47Z

src/dbnode/storage/series/buffer.go

 	if numStreams == 1 {
-		stream = streams[0]
+		mergedStream = streams[0]
 	} else {
 		// We may need to merge again here because the regular merge method does
 		// not merge buckets that have different versions.


It also doesn't merge warm and cold buckets basically right? maybe say that too

richardartoul · 2019-06-07T14:14:22Z

src/dbnode/storage/shard.go

@@ -1964,7 +1959,7 @@ func (s *dbShard) ColdFlush(
 			// Cold flushes can only happen on blockStarts that have been
 			// warm flushed, because warm flush logic does not currently
 			// perform any merging logic.
-			if !s.hasWarmFlushed(t.ToTime()) {
+			if !s.IsBlockRetrievable(t.ToTime()) {


Can you actually keep the duplicated method hasWarmFlushed? I know its lame to duplicate but I can easily see someone changing the meaning of IsBlockRetrievable and not expecting the impact it'll have on this

richardartoul

LGTM except for the last few minor comments

…cold flushed

justinjc requested review from richardartoul, robskillington and Haijuncao May 13, 2019 19:44

richardartoul reviewed May 14, 2019

View reviewed changes

justinjc force-pushed the juchan/cold-flush branch from 2ef92f3 to 39fafc1 Compare May 18, 2019 06:04

richardartoul reviewed May 22, 2019

View reviewed changes

justinjc changed the title ~~[WIP] [Do not land] Cold flush logic~~ [WIP] Cold flushes May 24, 2019

justinjc force-pushed the juchan/cold-flush branch 2 times, most recently from 74c32fd to aa3d985 Compare May 24, 2019 18:36

justinjc changed the title ~~[WIP] Cold flushes~~ Cold flushes May 27, 2019

justinjc requested a review from prateek May 28, 2019 14:00

justinjc mentioned this pull request May 28, 2019

Set up for compaction #1585

Closed

richardartoul reviewed May 28, 2019

View reviewed changes

prateek reviewed May 31, 2019

View reviewed changes

src/dbnode/storage/series/buffer.go Show resolved Hide resolved

prateek reviewed May 31, 2019

View reviewed changes

src/dbnode/storage/series/buffer.go Outdated Show resolved Hide resolved

prateek reviewed May 31, 2019

View reviewed changes

src/dbnode/storage/series/types.go Outdated Show resolved Hide resolved

prateek reviewed May 31, 2019

View reviewed changes

src/dbnode/persist/fs/merger.go Outdated Show resolved Hide resolved

robskillington reviewed Jun 1, 2019

View reviewed changes

src/dbnode/persist/fs/merger.go Show resolved Hide resolved

robskillington reviewed Jun 2, 2019

View reviewed changes

src/dbnode/persist/fs/merger.go Outdated Show resolved Hide resolved

robskillington reviewed Jun 2, 2019

View reviewed changes

src/dbnode/persist/fs/merger.go Show resolved Hide resolved

robskillington reviewed Jun 2, 2019

View reviewed changes

src/dbnode/persist/fs/merger.go Outdated Show resolved Hide resolved

robskillington reviewed Jun 2, 2019

View reviewed changes

richardartoul reviewed Jun 4, 2019

View reviewed changes

justinjc force-pushed the juchan/cold-flush branch from 89d446e to 8284612 Compare June 4, 2019 23:06

richardartoul reviewed Jun 6, 2019

View reviewed changes

src/dbnode/persist/fs/merger.go Show resolved Hide resolved

src/dbnode/persist/fs/merger.go Show resolved Hide resolved

src/dbnode/storage/shard.go Outdated Show resolved Hide resolved

richardartoul reviewed Jun 6, 2019

View reviewed changes

src/dbnode/storage/shard.go Show resolved Hide resolved

src/dbnode/storage/flush.go Show resolved Hide resolved

src/dbnode/storage/flush.go Show resolved Hide resolved

richardartoul reviewed Jun 7, 2019

View reviewed changes

src/dbnode/storage/shard.go Show resolved Hide resolved

richardartoul reviewed Jun 7, 2019

View reviewed changes

richardartoul approved these changes Jun 7, 2019

View reviewed changes

justinjc added 17 commits June 7, 2019 13:52

Cold flush logic

3b16ead

Only consider dirty cold buckets in considering if blocks need to be …

a213227

…cold flushed

Cold flush PR comments

f14581c

Fileset merge interface

3aa8c79

Rebase fixes

af036ee

Change interfaces to make code more testable; add tests

e0383de

Add merger test

47bd9b6

FetchBlocksForColdFlush returning no data is okay

3748a7b

Shard ColdFlush test

fe17ef5

More tests, comments, PR feedback

2943d5b

Don't snapshot if cold flush fails

acc1fb5

Pass block states down to get correct blocks to cold flush

945e9cd

Fix mergeWithMem test

81f02b1

Snapshots to take into account cold writes

94fd0a5

Fix test

d318ad9

Tags NoFinalize; additional comments

a115331

Regen mocks

4f27aef

justinjc force-pushed the juchan/cold-flush branch from 2b5b42c to 4f27aef Compare June 7, 2019 17:53

Fix TestForEachRemaining

117c6ff

justinjc merged commit c7be2a4 into master Jun 7, 2019

justinjc deleted the juchan/cold-flush branch June 7, 2019 18:50

justinjc mentioned this pull request Jun 10, 2019

Support for numbered filesets #1720

Merged

Betula-L mentioned this pull request Oct 25, 2019

Memory leak introduced by cold flush #2015

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cold flushes #1624

Cold flushes #1624

justinjc commented May 9, 2019 •

edited

Loading

codecov bot commented May 17, 2019 •

edited

Loading

richardartoul May 28, 2019

justinjc May 28, 2019

prateek May 31, 2019

justinjc Jun 2, 2019

prateek May 31, 2019

justinjc Jun 2, 2019 •

edited

Loading

prateek May 31, 2019

robskillington Jun 2, 2019

robskillington Jun 2, 2019

robskillington Jun 2, 2019

justinjc Jun 2, 2019

robskillington Jun 2, 2019

justinjc Jun 2, 2019 •

edited

Loading

robskillington Jun 2, 2019 •

edited

Loading

justinjc Jun 2, 2019

richardartoul Jun 4, 2019

CLAassistant commented Jun 5, 2019 •

edited

Loading

richardartoul Jun 7, 2019

richardartoul Jun 7, 2019

richardartoul Jun 7, 2019

justinjc Jun 7, 2019

richardartoul left a comment


		nextVersion := m.retriever.RetrievableBlockColdVersion(startTime) + 1

		tmpCtx := context.NewContext()

Cold flushes #1624

Cold flushes #1624

Conversation

justinjc commented May 9, 2019 • edited Loading

codecov bot commented May 17, 2019 • edited Loading

Codecov Report

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

justinjc Jun 2, 2019 • edited Loading

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

justinjc Jun 2, 2019 • edited Loading

Choose a reason for hiding this comment

robskillington Jun 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Jun 5, 2019 • edited Loading

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

richardartoul left a comment

Choose a reason for hiding this comment

justinjc commented May 9, 2019 •

edited

Loading

codecov bot commented May 17, 2019 •

edited

Loading

justinjc Jun 2, 2019 •

edited

Loading

justinjc Jun 2, 2019 •

edited

Loading

robskillington Jun 2, 2019 •

edited

Loading

CLAassistant commented Jun 5, 2019 •

edited

Loading