[dbnode] Series ref resolver #3316

soundvibe · 2021-03-03T09:15:52Z

What this PR does / why we need it:

This PR implements series ref resolver which writes new series asynchronously, reducing lock contention during db-node bootstrapping. So now bootstrappers, instead of writing new series immediately, are able to use resolver to retrieve series ref. Resolver ensures that given series will be written async (and it will wait for the write to complete if it's still not inserted) or it will be returned immediately if it already exists.
Since new series are now written asynchronously, writing new data using series ref ideally should be done when more series ref resolvers are accumulated, because otherwise each data write will wait for the background write process to complete. So in Commit log and Peers bootstrappers some code were updated to retain the same performance as it was before.

Special notes for your reviewer:

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

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

* master: [aggregator] Move placement checks to a background job (#3315) [tests] Remove a few more usages of NoOpAllBootstrapper from tests (#3314) [coordinator] make drop timestamp apply to the metric rather than specific rule (#3310) Update server.go (#3298) Fix Data Race in checkoutSeriesWithLock (#3300) [query] Allow configuration of placement options (#3304)

…urged.

optimize commit log snapshot load.

added UniqueIndex to SeriesRef.

removed unnecessary resolvers.

codecov · 2021-03-08T14:31:45Z

Codecov Report

Merging #3316 (19b4e4e) into master (19b4e4e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3316   +/-   ##
=======================================
  Coverage    72.6%    72.6%           
=======================================
  Files        1099     1099           
  Lines      103579   103579           
=======================================
  Hits        75210    75210           
  Misses      23210    23210           
  Partials     5159     5159

Flag	Coverage Δ
aggregator	`76.8% <0.0%> (ø)`
cluster	`84.9% <0.0%> (ø)`
collector	`84.3% <0.0%> (ø)`
dbnode	`79.2% <0.0%> (ø)`
m3em	`74.4% <0.0%> (ø)`
m3ninx	`73.6% <0.0%> (ø)`
metrics	`19.8% <0.0%> (ø)`
msg	`74.7% <0.0%> (ø)`
query	`66.9% <0.0%> (ø)`
x	`80.3% <0.0%> (ø)`

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

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 19b4e4e...535e786. Read the comment docs.

* master: (22 commits) Remove deprecated fields (#3327) Add quotas to Permits (#3333) [aggregator] Drop messages that have a drop policy applied (#3341) Fix NPE due to race with a closing series (#3056) [coordinator] Apply auto-mapping rules if-and-only-if no drop policies are in effect (#3339) [aggregator] Add validation in AddTimedWithStagedMetadatas (#3338) [coordinator] Fix panic in Ready endpoint for admin coordinator (#3335) [instrument] Config option to emit detailed Go runtime metrics only (#3332) [aggregator] Sort heap in one go, instead of iterating one-by-one (#3331) [pool] Add support for dynamic, sync.Pool backed, object pools (#3334) Enable PANIC_ON_INVARIANT_VIOLATED for tests (#3326) [aggregator] CanLead for unflushed window takes BufferPast into account (#3328) Optimize StagedMetadatas conversion (#3330) [m3msg] Improve message scan performance (#3319) [dbnode] Add reason tag to bootstrap retries metric (#3317) [coordinator] Enable rule filtering on prom metric type (#3325) Update m3dbnode-all-config.yml (#3204) [coordinator] Include Type in RollupOp.Equal (#3322) [coordinator] Simplify iteration logic of matchRollupTarget (#3321) [coordinator] Add rollup type to remove specific dimensions (#3318) ...

robskillington · 2021-03-09T14:52:57Z

src/dbnode/storage/bootstrap/bootstrapper/commitlog/source.go

@@ -28,11 +28,14 @@ import (
 	"sync"
 	"time"

+	"golang.org/x/sync/errgroup"


Perhaps move the other third party imports at the bottom to the top here with errgroup import?

robskillington · 2021-03-09T14:59:52Z

src/dbnode/storage/bootstrap/bootstrapper/commitlog/source.go

+	errs, _ := errgroup.WithContext(ctx.GoContext())
+	errs.Go(worker.readSeriesBlocks)
+	if err := s.loadBlocks(worker.dataCh, writeType); err != nil {
+		close(worker.dataCh)


Hm doesn't it look like this is closed on line 119?

Would be better to close it from one place, either in the worker itself always (with defer), or from the outside always.

Maybe just remove this and rely on the defer in the worker itself?

Agree. Refactored to use cancellable context so that readSeriesBlocks could be cancelled if loadBlocks() returns an error unexpectedly.

linasm

Left some comments (mostly nits and clarity suggestions).

src/dbnode/storage/bootstrap/util.go

src/dbnode/storage/shard.go

src/dbnode/storage/types.go

src/dbnode/storage/namespace_bootstrap_data_accumulator.go

src/dbnode/storage/shard.go

src/dbnode/storage/shard_test.go

src/dbnode/storage/bootstrap/bootstrapper/peers/source.go

* master: [dbnode] Remove unused shardBlockVolume (#3347) Fix new Go 1.15+ vet check failures (#3345) [coordinator] Add config option to make rollup rules untimed (#3343) [aggregator] Raw TCP Client write queueing/buffering refactor (#3342) [dbnode] Fail M3TSZ encoding on DeltaOfDelta overflow (#3329)

* master: [dtest] ns update/delete api (#3344)

src/dbnode/storage/shard.go

linasm

LGTM. Any thoughts on putting storage.seriesResolver into its own file rather than further growing this monster shard.go file (see #3316 (comment))?

robskillington · 2021-03-12T20:24:43Z

I'd also support moving it into it's own file too

robskillington · 2021-03-12T20:25:10Z

src/dbnode/storage/bootstrap/bootstrapper/commitlog/source.go

+		tags.Close()
+
+		select {
+		case <-ctx.Done():


This might be expensive to check on every single series (since reading from channel does need thread safety), perhaps we can check this every N series? Say every 1024 just using a top level var here if i%1024==0 { /* check ctx.Done() */ }?

done: https://github.com/m3db/m3/pull/3316/files#diff-8ac680b720a386924c879c03fad30c39581359a9b8fcad59814ccab0399703b0R164

robskillington

LGTM other than minor comments (which would be good to address before merging, but stamping so that you can address those asynchronously and then merge PR)

… every time.

* master: [dbnode] Remove unused getMockReader (#3351) [dbnode] Add back teardown of containers at end of Prometheus integration test (#3349) [query] Add series metadata label limit header for trimming response (#3348) # Conflicts: # src/dbnode/storage/shard_test.go

* master: [dbnode] Fix clock options not propagated where needed (#3353)

src/dbnode/storage/shard.go

src/dbnode/storage/series_resolver.go

* master: [x] Add OffsetClock constructor that uses given time delta (#3354)

robskillington and others added 17 commits March 2, 2021 11:48

WIP [dbnode] Use series read write ref resolver

0777cc4

fixing the build and making unit tests pass.

fa01bbb

trying to fix resolve error.

1385c2f

trying to fix resolve error (2).

9cc2a22

trying to fix resolve error (3).

8c02b88

add logging to check if series was expired.

48fdcf8

try incrementing reader-writer count for a new entry so it won't be p…

537823a

…urged.

handle accumulator close errors.

abf5160

optimize commit log snapshot load.

test commit with warm flush and seriesId cache.

ef4d08d

trying to fix warm flush.

87979c8

revert flush changes.

b404219

use multiErrors.

acc2f0a

fixed index write when using series ref.

c0877e8

added UniqueIndex to SeriesRef.

insert new entry and inc ref count.

80f4e42

fixed linter issues.

b481a6c

small cleanup.

6078828

removed unnecessary resolvers.

soundvibe added 2 commits March 9, 2021 11:13

optimized series ref resolver usage.

2f5f2c8

added unit test.

05bec35

soundvibe changed the title ~~[WIP] [dbnode] Series ref resolver~~ [dbnode] Series ref resolver Mar 9, 2021

soundvibe added 3 commits March 9, 2021 15:37

fixing possible race.

629efb9

simplified error handling in commit log bs.

28caa33

soundvibe requested review from robskillington, notbdu and linasm March 9, 2021 14:26

robskillington reviewed Mar 9, 2021

View reviewed changes

linasm self-assigned this Mar 9, 2021

linasm reviewed Mar 10, 2021

View reviewed changes

linasm removed their assignment Mar 10, 2021

soundvibe added 4 commits March 10, 2021 13:04

changes after review.

51ed40f

removed previously added logging line for testing.

be6604b

updated mocks.

baeca8d

updated some code comments.

d3c69cf

soundvibe requested a review from linasm March 10, 2021 12:02

soundvibe assigned linasm Mar 10, 2021

Merge branch 'master' into linasn/series-resolver

2d14706

* master: [dtest] ns update/delete api (#3344)

soundvibe requested review from notbdu and robskillington March 10, 2021 13:10

linasm reviewed Mar 10, 2021

View reviewed changes

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

renamed entryRefCountIncremented option to releaseEntryRef.

bfc972a

linasm reviewed Mar 12, 2021

View reviewed changes

robskillington reviewed Mar 12, 2021

View reviewed changes

robskillington approved these changes Mar 12, 2021

View reviewed changes

soundvibe added 4 commits March 15, 2021 11:53

extracted series resolver into separate file.

15b82e1

check if context was not cancelled once in 1024 iterations instead of…

49561ad

… every time.

fixed comment.

2d2fc76

soundvibe requested a review from linasm March 15, 2021 12:18

Merge branch 'master' into linasn/series-resolver

4489d21

* master: [dbnode] Fix clock options not propagated where needed (#3353)

linasm reviewed Mar 15, 2021

View reviewed changes

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

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

soundvibe added 2 commits March 15, 2021 15:05

inline retrieveWritableSeries function into series resolver.

49f9753

Merge branch 'master' into linasn/series-resolver

535e786

* master: [x] Add OffsetClock constructor that uses given time delta (#3354)

soundvibe merged commit 3521fd7 into master Mar 15, 2021

soundvibe deleted the linasn/series-resolver branch March 15, 2021 13:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dbnode] Series ref resolver #3316

[dbnode] Series ref resolver #3316

soundvibe commented Mar 3, 2021 •

edited

Loading

codecov bot commented Mar 8, 2021 •

edited

Loading

robskillington Mar 9, 2021

soundvibe Mar 10, 2021

robskillington Mar 9, 2021

robskillington Mar 9, 2021

soundvibe Mar 10, 2021

linasm left a comment

linasm left a comment

robskillington commented Mar 12, 2021

robskillington Mar 12, 2021 •

edited

Loading

soundvibe Mar 15, 2021

robskillington left a comment

[dbnode] Series ref resolver #3316

[dbnode] Series ref resolver #3316

Conversation

soundvibe commented Mar 3, 2021 • edited Loading

codecov bot commented Mar 8, 2021 • edited Loading

Codecov Report

robskillington Mar 9, 2021

Choose a reason for hiding this comment

soundvibe Mar 10, 2021

Choose a reason for hiding this comment

robskillington Mar 9, 2021

Choose a reason for hiding this comment

robskillington Mar 9, 2021

Choose a reason for hiding this comment

soundvibe Mar 10, 2021

Choose a reason for hiding this comment

linasm left a comment

Choose a reason for hiding this comment

linasm left a comment

Choose a reason for hiding this comment

robskillington commented Mar 12, 2021

robskillington Mar 12, 2021 • edited Loading

Choose a reason for hiding this comment

soundvibe Mar 15, 2021

Choose a reason for hiding this comment

robskillington left a comment

Choose a reason for hiding this comment

soundvibe commented Mar 3, 2021 •

edited

Loading

codecov bot commented Mar 8, 2021 •

edited

Loading

robskillington Mar 12, 2021 •

edited

Loading