[dbnode] Add forward index write capability #1613

arnikola · 2019-05-08T16:58:05Z

What this PR does / why we need it:
This PR eases index pressure at block boundaries by allowing incoming writes near the boundary to have a chance to be dual written to the upcoming index block based on config defined probabilities.

Special notes for your reviewer:
Ensure that there's no further changes to timestamp required.

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

…s correctly written to the future block

richardartoul

Could you wire the configuration for this feature up to the YAML and add some documentation about it (comment over the YAML or markdown or w/e)

Also, are there any implications on correctness here? I.E I think this feature may impact the results of aggregation queries. Thats probably fine but worth documenting

richardartoul · 2019-05-09T01:14:50Z

src/dbnode/storage/forward_index_dice.go

+
+// forwardIndexDice is a die roll that adds a chance for incoming index writes
+// arriving near a block boundary to be duplicated and written to the next block
+// index, adding jitter and smoothing index load.


"and smoothing index load so that block boundaries do not cause a huge influx of new documented that need to be indexed all at once"

src/dbnode/storage/forward_index_dice.go

richardartoul · 2019-05-09T01:19:01Z

src/dbnode/storage/forward_index_dice.go

+			fmt.Errorf("invalid forward write threshold %f", threshold)
+	}
+
+	bufferFragment := float64(bufferFuture) * threshold


I don't understand why there is a relationship between bufferFuture and the threshold...I guess in my mind bufferFuture would set a maximum on the threshold so if bufferFuture is 10 minutes and you have a block size of 2 hours then the earliest you could start forward indexing would be at 1:50PM, but I don't understand the logic here where you multiply bufferFuture by some threshold

Yeah, so threshold is defined as a fragment of the bufferFuture to allow some tweaking on when ; my thinking is you'd put the threshold at 0.9 or something, and then in your example, we'd start forward indexing at 1:51 PM, just to cut down on any possibility of trying to write the values too early in case of positive skew on the emitter.

i.e. bufferFragment ∈ (0, bufferFuture]

mind adding an explanation for this in a comment

src/dbnode/storage/forward_index_dice_test.go

richardartoul · 2019-05-09T01:22:13Z

src/x/dice/dice.go

@@ -0,0 +1,61 @@
+// Copyright (c) 2019 Uber Technologies, Inc.


I coulda sworn we already had a dice package that we use in a few different places. Is this just coming from m3x?

This is from internal statsdex; I think there's a sampler package we have already, but it performs slightly differently (i.e. NewSampler(0.1).Sample() returns true IFF it's been called a multiple of 10 times)

richardartoul · 2019-05-09T01:23:33Z

src/x/dice/dice.go

+}
+
+func (d *epoch) Roll() bool {
+	return (time.Now().UnixNano()+d.jitter)%d.r == 0


out of curiosity, why time over a rand generator? Thread safety?

@arnikola could we use pcg (similar to how we do here) instead of time.Now(). Will still seed with time.Now() so won't affect randomness but will be a lot quicker (7ns v 50ns).

richardartoul · 2019-05-09T01:24:22Z

src/x/dice/dice.go

+	Roll() bool
+}
+
+// NewEpochDice constructs a new Dice based on UNIX epoch time.


Can you clarify this comment to explain that it uses the current time as a source of randomness

richardartoul · 2019-05-09T01:29:50Z

src/dbnode/storage/index.go

@@ -524,8 +539,15 @@ func (i *nsIndex) writeBatches(
 				batch.MarkUnmarkedEntryError(m3dberrors.ErrTooPast, idx)
 				return
 			}
+
+			if i.forwardIndexDice.forwardIndexing(entry.Timestamp) {
+				forwardWriteEntry := entry


Is this a safe clone? There are three fields that are "pointer" type:

OnIndexSeries

result

resultVal

Regardless I wonder if its worth adding a clone() method so that even if this operation is safe now its more likely to stay safe in the future

richardartoul · 2019-05-09T01:31:55Z

src/dbnode/storage/index.go

+
+			if i.forwardIndexDice.forwardIndexing(entry.Timestamp) {
+				forwardWriteEntry := entry
+				forwardWriteEntry.Timestamp = entry.Timestamp.Add(blockSize)


Is the timestamp used for anything other than figuring out which block to put it in? Also, I may be wrong but my understanding of the forward indexing logic seemed to imply that you could forward index stuff that wasn't yet within the bufferFuture window. Is that correct? If so, is it guaranteed there is a block available for this write to get indexed into?

Given the current behaviour, it will only forward index within a percentage of the bufferFuture window

prateek · 2019-05-12T22:37:30Z

src/dbnode/storage/index.go

 		})

+	batch.AppendAll(forwardWriteBatch)


Unfortunately this isn't safe as is. When trying to index a Series, we track the last successful block starts when it was indexed (in *lookup.Entry via the shard map); and further, maintain a reference count to Series entry. Both of these are updated via the OnIndexSeries per element in the WriteBatchEntry.

By just appending entries the way it's being done right now, it's going to definitely mess up the reference count (because you'll dec for each copy, even though we only have a inc on the original).

Won't necessarily mess up the last successful write times but probably need to consider some nuances there too.

Re: reference counting - we can either inc the appropriate references when making copies (safest), or ensure we only dec the final copy amongst all the ones we make here (can't be the original, has to be last one we track because of cleanup semantics in the Shard).

Re: tracking last successful write time update. Here's a good explanation.

AFAICT, the changes you're introducing can make it so that we're writing to 4 block starts (instead of the 3 we currently assume worst case). I'm not a 100% sure that's accurate tho. Should work that out, and if so it makes sense to bump the size of the static alloc from 3 to 4.

codecov · 2019-05-28T17:14:28Z

Codecov Report

Merging #1613 into master will increase coverage by <.1%.
The diff coverage is 97%.

@@           Coverage Diff            @@
##           master   #1613     +/-   ##
========================================
+ Coverage    71.9%     72%   +<.1%     
========================================
  Files         954     956      +2     
  Lines       79170   79236     +66     
========================================
+ Hits        56989   57057     +68     
+ Misses      18460   18454      -6     
- Partials     3721    3725      +4

Flag	Coverage Δ
#aggregator	`82.3% <ø> (ø)`	⬆️
#cluster	`85.7% <ø> (ø)`	⬆️
#collector	`63.7% <ø> (ø)`	⬆️
#dbnode	`79.9% <96.4%> (ø)`	⬆️
#m3em	`73.2% <ø> (ø)`	⬆️
#m3ninx	`73.9% <ø> (-0.1%)`	⬇️
#m3nsch	`51.1% <ø> (ø)`	⬆️
#metrics	`17.6% <ø> (ø)`	⬆️
#msg	`74.7% <ø> (ø)`	⬆️
#query	`67.1% <ø> (-0.1%)`	⬇️
#x	`87.1% <100%> (-0.2%)`	⬇️

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 d75fc7f...fd50349. Read the comment docs.

prateek · 2019-05-29T15:40:03Z

src/x/dice/dice.go

+}
+
+func (d *epoch) Roll() bool {
+	return rand.Int63()%d.r == 0


Avoid using the global rand, it's a locked source: https://golang.org/src/math/rand/rand.go?s=9470:9488#L288. Better to control whatever concurrency you need by yourself.

Could you also use the github.com/MichaelTJones/pcg RNG. It's a lot quicker/cheaper than the standard one.

prateek · 2019-05-29T15:50:26Z

src/dbnode/storage/forward_index_dice.go

+}
+
+// forwardIndexing decides if a timestamp is eligible for forward index writes.
+func (o *forwardIndexDice) forwardIndexing(timestamp time.Time) bool {


nit: rename to something like shouldIndex or roll to indicate it's a test. the current method name doesn't convey what you're testing for

SGTM, changed to roll to preserve dice context

prateek · 2019-05-29T15:52:20Z

src/dbnode/storage/index.go

@@ -524,8 +539,23 @@ func (i *nsIndex) writeBatches(
 				batch.MarkUnmarkedEntryError(m3dberrors.ErrTooPast, idx)
 				return
 			}
+
+			if i.forwardIndexDice.forwardIndexing(entry.Timestamp) {


once you fix the global rand usage, will need to be careful about using this dice

if we already have a lock that we can use to protect the dice, than it's gucci. if not, will have to think about the contention it'll introduce to add a lock here.

I'll see if there's a way to avoid having to get a lock here; there's a lock above batch processing I could roll in, but that would mean that the forward write would be applied to the whole batch. If that's fine, should be easy 👍

prateek · 2019-05-29T19:45:51Z

src/dbnode/storage/index/block_bench_test.go

+
+}
+
+func (m mockOnIndexSeries) NeedsIndexUpdate(indexBlockStartForWrite xtime.UnixNano) bool {


can you just return false always? doesn't seem like you're updating the value ever

Yeah fair, had it like this in case we wanted to be able to return different values in the future

prateek · 2019-05-30T18:00:38Z

src/dbnode/storage/index_queue_forward_write_test.go

+	reQuery, err := m3ninxidx.NewRegexpQuery([]byte("name"), []byte("val.*"))
+	assert.NoError(t, err)
+
+	// NB: query both the current and the next index block to ensure that the


I don't think this is confirming that the write exists in the next block -- cause you're querying both current and next block. Need to ensure you're checking both individually, no?

It should be fine, now is 5 minutes before the block boundary, and this will check the range which is from 6 to 4 minutes before the boundary to check for the current block, and then 6 to 4 minutes before the next block boundary to check only the future block

prateek · 2019-05-30T18:05:48Z

src/dbnode/storage/index_queue_forward_write_test.go

+	reQuery, err := m3ninxidx.NewRegexpQuery([]byte("name"), []byte("val.*"))
+	assert.NoError(t, err)
+
+	// NB: query both the current and the next index block to ensure that the


Should be same here

prateek · 2019-05-30T18:06:28Z

src/dbnode/storage/index_queue_forward_write_test.go

+		lifecycle = index.NewMockOnIndexSeries(ctrl)
+	)
+
+	lifecycle.EXPECT().OnIndexFinalize(xtime.ToUnixNano(ts))


super nit: mind putting these in gomock.InOrder(...) as much as possible

prateek

LGTM save the in order nit

prateek · 2019-05-30T19:10:21Z

src/dbnode/storage/index.go

 				batch.MarkUnmarkedEntryError(m3dberrors.ErrTooPast, idx)
 				return
 			}
+
+			if forwardIndexDice.roll(ts) {


should this also be if forwardIndexEnabled && ..

maybe add a test that would have caught this

The !enabled case should be caught here: https://github.com/m3db/m3/pull/1613/files/70b0912726891694461e47b7d21499aeddb8bbca#diff-b5380446b59ebd51b8b8a482df15c4d7R93

[dbnode] Add forward index write capability

70a6275

arnikola requested review from prateek, robskillington and richardartoul May 8, 2019 16:58

Improved the forward write test to check that a valid forward write i…

5ab7d96

…s correctly written to the future block

richardartoul reviewed May 9, 2019

View reviewed changes

prateek reviewed May 12, 2019

View reviewed changes

arnikola added 4 commits May 22, 2019 14:56

Adding index lifecycle hooks

dd6a46f

Adding file

c97b039

Add remaining forward index tests

2c088de

Fix dice test

9552b74

arnikola added 3 commits May 28, 2019 15:03

Fix mockOnIndexSeries

fd50349

Merge branch 'master' into arnikola/forward-index

27904c3

Fix merge issues

644e4e5

prateek reviewed May 29, 2019

View reviewed changes

arnikola added 2 commits May 30, 2019 11:20

Merge branch 'master' into arnikola/forward-index

242ffa4

WIP

83042b7

prateek reviewed May 30, 2019

View reviewed changes

prateek approved these changes May 30, 2019

View reviewed changes

Test clarifications, ordering

70b0912

arnikola merged commit f4c2694 into master May 30, 2019

arnikola deleted the arnikola/forward-index branch May 30, 2019 19:09

prateek reviewed May 30, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dbnode] Add forward index write capability #1613

[dbnode] Add forward index write capability #1613

arnikola commented May 8, 2019

richardartoul left a comment

richardartoul May 9, 2019

richardartoul May 9, 2019

arnikola May 9, 2019

prateek May 29, 2019

richardartoul May 9, 2019

arnikola May 9, 2019

richardartoul May 9, 2019

prateek May 12, 2019

richardartoul May 9, 2019

richardartoul May 9, 2019

richardartoul May 9, 2019

arnikola May 9, 2019

prateek May 12, 2019

prateek May 12, 2019

codecov bot commented May 28, 2019 •

edited

Loading

prateek May 29, 2019

prateek May 29, 2019

arnikola May 29, 2019

prateek May 29, 2019

prateek May 29, 2019

arnikola May 29, 2019

prateek May 29, 2019

arnikola May 30, 2019

prateek May 30, 2019

arnikola May 30, 2019

prateek May 30, 2019

arnikola May 30, 2019

prateek May 30, 2019

prateek left a comment

prateek May 30, 2019

arnikola May 30, 2019

		@@ -0,0 +1,61 @@
		// Copyright (c) 2019 Uber Technologies, Inc.


		}

		func (m mockOnIndexSeries) NeedsIndexUpdate(indexBlockStartForWrite xtime.UnixNano) bool {

[dbnode] Add forward index write capability #1613

[dbnode] Add forward index write capability #1613

Conversation

arnikola commented May 8, 2019

richardartoul 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 28, 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

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

prateek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 28, 2019 •

edited

Loading