Delete commit log files that are covered by snapshot files #802

richardartoul · 2018-07-23T18:11:11Z

Refactors how we list / interact with the commit log files on disk (specifically, we no longer use the name of the file to determine the start time nor do we rely on the current configuration to guess what the commit log block size is)
Rewrite commit log cleanup logic to allow commit logs to be deleted if they're captured by snapshots
In addition to the standard unit tests that would be expected with this P.R, I've added a mixed mode read write integration prop test that uses a random seed and property testing to verify that the cleanup logic always leave the node in a state where if its restarted all data can be recovered.

codecov · 2018-07-23T18:18:32Z

Codecov Report

Merging #802 into master will increase coverage by 0.04%.
The diff coverage is 92.43%.

@@            Coverage Diff             @@
##           master     #802      +/-   ##
==========================================
+ Coverage   77.98%   78.02%   +0.04%     
==========================================
  Files         365      365              
  Lines       31438    31479      +41     
==========================================
+ Hits        24516    24561      +45     
+ Misses       5262     5258       -4     
  Partials     1660     1660

Flag	Coverage Δ
#coordinator	`60.14% <ø> (-0.1%)`	⬇️
#dbnode	`81.56% <92.43%> (+0.06%)`	⬆️
#m3ninx	`72.7% <ø> (ø)`	⬆️

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 c096772...a68bb16. Read the comment docs.

robskillington · 2018-07-25T20:38:50Z

src/dbnode/integration/fs_commitlog_snapshot_mixed_mode_read_write_prop_test.go

+const maxBlockSize = 12 * time.Hour
+const maxPoints = 100
+const minSuccessfulTests = 8
+const maxFlushWaitTime = time.Minute


nit: combine into single const block:

const ( minBlockSize = 15 * time.Minute maxBlockSize = 12 * time.Hour maxPoints = 100 minSuccessfulTests = 8 maxFlushWaitTime = time.Minute )

robskillington · 2018-07-25T20:42:00Z

src/dbnode/persist/fs/commitlog/files.go

+
+// Files returns a slice of all available commit log files on disk along with
+// their associated metadata.
+func Files(opts Options) ([]File, error) {


robskillington · 2018-07-25T20:43:51Z

src/dbnode/persist/fs/commitlog/iterator.go

-			continue
-		}
-		if predicate(file, start, duration) {
+		if predicate(file.FilePath, file.Start, file.Duration) {


Would it be better to make predicate just take commitlog.File so it's easier to add new fields in the future?

robskillington · 2018-07-25T20:48:32Z

src/dbnode/storage/cleanup.go

+			var (
+				ropts                      = ns.Options().RetentionOptions()
+				nsBlocksStart, nsBlocksEnd = commitLogNamespaceBlockTimes(start, duration, ropts)
+				needsFlush                 = ns.NeedsFlush(nsBlocksStart, nsBlocksEnd)


Hm, I wonder if we should change these type of functions to instead of taking start, end time.Time we should be using a single xtime.Range arg?

not sure how much it buys us as all the helper functions take a start/end as well and we're not trying to calculate overlap or anything like that

Yeah, just more that the name of the data structure represents the variable itself better and its a single var that can then be passed around. I'm not bullish though, we can revisit this later.

cool lets leave this out in the interest of time for now

robskillington · 2018-07-25T20:50:20Z

src/dbnode/storage/cleanup.go

+	filesToCleanup := filterCommitLogFiles(files, func(start time.Time, duration time.Duration) bool {
+		if outerErr != nil {
+			return false
+		}


Is this a necessary early return false? It seems like we'll reach same result whether we skip entries after we've seen a single entry. Just because if so, then we don't have to add test coverage for these lines if it doesn't make a difference.

Yeah I guess its just the difference between first error wins / last error wins. Some of the operations are expensive in that they have to go to disk so returning early is nice in that sense too, but optimizing for the error case is kind of dumb. I'll remove it.

robskillington · 2018-07-26T03:11:38Z

src/dbnode/integration/disk_cleanup_multi_ns_test.go

+		writeCommitLogDataSpecifiedTS(
+			t,
+			testSetup,
+			testSetup.storageOpts.CommitLogOptions().SetFlushInterval(defaultIntegrationTestFlushInterval),


super nit: Maybe declare these options before the for loop? Just to avoid this super long line and all:

clOpts := testSetup.storageOpts.CommitLogOptions(). SetFlushInterval(defaultIntegrationTestFlushInterval) for _, clTime := range commitLogTimes { // Need to generate valid commit log files otherwise cleanup will fail. data := map[xtime.UnixNano]generate.SeriesBlock{ xtime.ToUnixNano(clTime): nil, } writeCommitLogDataSpecifiedTS(t, testSetup, clOpts, data, ns1, clTime, false) }

robskillington · 2018-07-26T03:13:01Z

src/dbnode/integration/disk_cleanup_test.go

+		writeCommitLogDataSpecifiedTS(
+			t,
+			testSetup,
+			testSetup.storageOpts.CommitLogOptions().SetFlushInterval(defaultIntegrationTestFlushInterval),


super nit: Same here.

robskillington · 2018-07-26T03:16:18Z

src/dbnode/integration/integration_data_verify.go

@@ -73,7 +74,7 @@ func verifySeriesMapForRange(
 	input generate.SeriesBlock,
 	expectedDebugFilePath string,
 	actualDebugFilePath string,
-) {
+) bool {


Hm this is probably a good change. Especially when called in an async goroutine because otherwise the goroutine just dies when a require.Foo(...) fails. 👍

yeah plus gopter require you to return (false, error) when something goes wrong. If you just panic it can't really do its thing

robskillington · 2018-07-26T03:18:21Z

src/dbnode/storage/cleanup.go

+			isCapturedBySnapshot, err := ns.IsCapturedBySnapshot(
+				nsBlocksStart, nsBlocksEnd, start.Add(duration))
+			if err != nil {
+				outerErr = err


If you want to get the first error btw, why not make it a slice var outerErrs []error then outerErrs = append(outerErrs, err) and then if len(outerErrs) > 0 { return outerErrs[0] }?

Or even just use a multiErr here and return all errors shrug.

I realized that I could just make the filterCommitLogFiles accept a predicate function that returns (bool, error) and that makes everything clean and simple.

robskillington

LGTM

richardartoul changed the title ~~[WIP] - Delete commit log files that are covered by snapshot files~~ Delete commit log files that are covered by snapshot files Jul 24, 2018

Squash

8a33ff2

richardartoul force-pushed the ra/delete-snapshot-files-master branch from b7173e9 to 8a33ff2 Compare July 24, 2018 21:47

Richard Artoul added 10 commits July 24, 2018 17:48

Fix broken tests

1e2573d

Reduce max points to 100

d14b3f5

Delete TODO

c81d092

Remove unnecessary nil check

b3efd63

Fix panic in integration test

9d2f4fb

Fix broken integration test

74920dd

Fix typo

ab9a2ab

Fix typo

13be077

Fix typo

cf60364

undelete cod

a4b4f8e

robskillington reviewed Jul 25, 2018

View reviewed changes

Richard Artoul added 3 commits July 25, 2018 17:41

Address Robs feedback

632adef

Fix prop test

97747fe

fix compilation issue

4f96547

robskillington reviewed Jul 26, 2018

View reviewed changes

Richard Artoul added 4 commits July 26, 2018 10:15

cleanup function calls

dd6c1ed

Refactor how we filter commit log files and handle errors

f09ef58

Fix compulation error

5ce1e6d

Fix compulation issue

134375b

robskillington approved these changes Jul 26, 2018

View reviewed changes

Fix bug in cleanup logic

a68bb16

richardartoul merged commit 8968605 into master Jul 26, 2018

justinjc deleted the ra/delete-snapshot-files-master branch August 9, 2018 18:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete commit log files that are covered by snapshot files #802

Delete commit log files that are covered by snapshot files #802

richardartoul commented Jul 23, 2018 •

edited

Loading

codecov bot commented Jul 23, 2018 •

edited

Loading

robskillington Jul 25, 2018 •

edited

Loading

richardartoul Jul 25, 2018

robskillington Jul 25, 2018

robskillington Jul 25, 2018

richardartoul Jul 25, 2018

robskillington Jul 25, 2018

richardartoul Jul 25, 2018

robskillington Jul 26, 2018

richardartoul Jul 26, 2018

robskillington Jul 25, 2018 •

edited

Loading

richardartoul Jul 25, 2018

robskillington Jul 26, 2018 •

edited

Loading

richardartoul Jul 26, 2018

robskillington Jul 26, 2018

richardartoul Jul 26, 2018

robskillington Jul 26, 2018

richardartoul Jul 26, 2018

robskillington Jul 26, 2018

robskillington Jul 26, 2018

richardartoul Jul 26, 2018

robskillington left a comment

Delete commit log files that are covered by snapshot files #802

Delete commit log files that are covered by snapshot files #802

Conversation

richardartoul commented Jul 23, 2018 • edited Loading

codecov bot commented Jul 23, 2018 • edited Loading

Codecov Report

robskillington Jul 25, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robskillington Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robskillington Jul 26, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robskillington left a comment

Choose a reason for hiding this comment

richardartoul commented Jul 23, 2018 •

edited

Loading

codecov bot commented Jul 23, 2018 •

edited

Loading

robskillington Jul 25, 2018 •

edited

Loading

robskillington Jul 25, 2018 •

edited

Loading

robskillington Jul 26, 2018 •

edited

Loading