[dbnode] Add claims for index segments volume index #2846

robskillington · 2020-11-06T08:04:12Z

What this PR does / why we need it:

This adds the ability to safely flush in parallel for index segments for the same block start without concern about concurrency and writing to the same file paths. Adds ability to safely do more compactions/updates/etc to same block start for index segments.

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

notbdu

Left a couple nits, but changes LGTM.

Seems like this logic works fine w/ how we determine whether we've warm flushed an index block:

func (i *nsIndex) hasIndexWarmFlushedToDisk(
        infoFiles []fs.ReadIndexInfoFileResult,
        blockStart time.Time,
) bool {
        var hasIndexWarmFlushedToDisk bool
        // NB(bodu): We consider the block to have been warm flushed if there are any
        // filesets on disk. This is consistent with the "has warm flushed" check in the db shard.
        // Shard block starts are marked as having warm flushed if an info file is successfully read from disk.
        for _, f := range infoFiles {
                indexVolumeType := idxpersist.DefaultIndexVolumeType
                if f.Info.IndexVolumeType != nil {
                        indexVolumeType = idxpersist.IndexVolumeType(f.Info.IndexVolumeType.Value)
                }
                if f.ID.BlockStart == blockStart && indexVolumeType == idxpersist.DefaultIndexVolumeType {
                        hasIndexWarmFlushedToDisk = true
                }
        }
        return hasIndexWarmFlushedToDisk
}

We check the index volume type so there is no dependency on the volume index.

notbdu · 2020-11-06T16:12:13Z

src/dbnode/persist/fs/persist_manager.go

+	// Now check if previous claim exists.
+	blockStart := xtime.ToUnixNano(blockStartTime)
+	namespace := namespaceMetadata.ID()
+	key := fmt.Sprintf("%s/%s/%d", filePathPrefix, namespace.String(),


nit: is it better to have a nested map here so we're not generating a ton of keys with duplicated strings for long retention?

Also, isn't file path prefix static after set? I don't think we have multiple file paths and/or dynamically changing file paths at runtime?

notbdu · 2020-11-06T16:14:50Z

src/dbnode/persist/fs/persist_manager.go

+	rp, bs, t := retOpts.RetentionPeriod(), indexOpts.BlockSize(), nowFn()
+	earliestBlockStart := retention.FlushTimeStartForRetentionPeriod(rp, bs, t)
+	earliestBlockStartUnixNanos := xtime.ToUnixNano(earliestBlockStart)
+	for key, claim := range indexVolumeIndexClaims {


nit: prob a hyper optimization but would it be better to just iterate over block starts beginning w/ edge of retention and going backwards in time until we can't find any claim and bail then? That way we're only ever iterating over block starts that are out of retention (if any). If there are none, it will just check the map once and bail early.

notbdu · 2020-11-06T16:16:23Z

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

+			// to validate the index segment.
+			// If fails validation will rebuild since missing from
+			// fulfilled range.
+			fsOpts = fsOpts.SetIndexReaderAutovalidateIndexSegments(true)


+1 on this change, are there other places that we'd want to also validate index segments on read or are we already doing it?

Self f/u on this: It looks like the only other place we are reading in index segments is in the persist manager after flush the segments to disk. We've just written to disk at that point so that should be fine.

nbroyles

LG pending code gen + test fix ups

notbdu · 2020-11-06T16:37:37Z

src/dbnode/persist/fs/persist_manager.go

@@ -654,3 +657,70 @@ func (pm *persistManager) SetRuntimeOptions(value runtime.Options) {
 	pm.currRateLimitOpts = value.PersistRateLimitOptions()
 	pm.Unlock()
 }
+
+var (
+	indexVolumeIndexClaimsLock sync.Mutex


If we decide on going down the path of a global persist manager instance vs a global shared lock btwn persist managers, currently both flush and cold flush each have their own persist manager instance.

We also create one for the fs/peers bootstrappers but they run serially.

Spoke offline, decided on going w/ a global index volume index claims manager approach instead.

nbroyles · 2020-11-10T15:25:50Z

src/dbnode/persist/fs/index_claims_manager.go

+
+// NewIndexClaimsManager returns an instance of the index claim manager. This manages
+// concurrent claims for volume indices per ns and block start.
+// NB(bodu): There should be only a single shared index claim manager among all threads


nit: does it make sense to make this method cache an instance and just return one if it already exists?

Hmm so there are unit tests that rely on the CTOR returning separate instances of the index claims manager.

I could add a reset method to the index claims manager and explicitly reset before each test but that feels kinda awkward esp just for tests...

I think it's better to just keep it as is for now. We have some confidence we're using a single instance since we are setting it at the storage.Options level and propagating it through the db w/ validation checks if not set.

nbroyles · 2020-11-10T15:37:21Z

Still LG @notbdu. Codegen failure looks lint related so nbd. 👍

…letion of out of retention claims.

codecov · 2020-11-10T23:19:53Z

Codecov Report

Merging #2846 (f1ededb) into master (5b5c050) will increase coverage by 0.0%.
The diff coverage is 83.6%.

@@           Coverage Diff            @@
##           master    #2846    +/-   ##
========================================
  Coverage    72.1%    72.1%            
========================================
  Files        1100     1101     +1     
  Lines       99968   100071   +103     
========================================
+ Hits        72077    72184   +107     
+ Misses      22941    22933     -8     
- Partials     4950     4954     +4

Flag	Coverage Δ
aggregator	`75.8% <ø> (-0.1%)`	⬇️
cluster	`85.0% <ø> (ø)`
collector	`84.3% <ø> (ø)`
dbnode	`79.2% <83.6%> (+<0.1%)`	⬆️
m3em	`74.4% <ø> (ø)`
m3ninx	`73.1% <ø> (ø)`
metrics	`17.2% <ø> (ø)`
msg	`74.0% <ø> (-0.1%)`	⬇️
query	`68.8% <ø> (ø)`
x	`80.6% <ø> (+0.2%)`	⬆️

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 5b5c050...f1ededb. Read the comment docs.

* master: (28 commits) [dbnode] Add claims for index segments volume index (#2846) [dbnode] Remove namespaces from example config and integration tests (#2866) [dbnode] Resurrect flaky test skip (#2868) [aggregator] Fix checkCampaignStateLoop (#2867) [dbnode] implement deletion method in namespace kvadmin service (#2861) Replace closer with resource package (#2864) Add coding style guide (#2831) Add GOVERNANCE.md to describe governance (#2830) Add COMPATIBILITY.md to describe version compatibility (#2829) Refactor etcd config as discovery section with convenience types (#2843) Refactor x/lockfile into dbnode/server (#2862) [lint] Disable nlreturn linter (#2865) [m3cluster] Expose placement algorithm in placement service (#2858) [etcd] Set reasonable cluster connection/sync settings by default (#2860) [dbnode] Use bits.LeadingZeros64 to improve encoder performance (#2857) Cleanup m3nsch leftovers (#2856) Update ci-scripts to correct coverage tracking (#2854) [aggregator] Process campaign state without waiting for first campaign check interval (#2855) Bump go to 1.14 (#2853) [query] Remove single series error from M3 ...

notbdu approved these changes Nov 6, 2020

View reviewed changes

nbroyles self-requested a review November 6, 2020 16:22

nbroyles approved these changes Nov 6, 2020

View reviewed changes

notbdu reviewed Nov 6, 2020

View reviewed changes

notbdu force-pushed the r/add-claims-for-index-segment-volume-indexes branch 2 times, most recently from 7bda086 to a8e0720 Compare November 9, 2020 19:57

nbroyles reviewed Nov 10, 2020

View reviewed changes

robskillington and others added 13 commits November 10, 2020 16:13

[dbnode] Add claims for index segments volume index

ae4824c

Update bootstrap.go

7995a26

Fix codegen

92c5f73

Fix unit tests.

25ce9e7

Add and use index claims manager.

c1617a6

Fix unit tests.

bb6f691

Rename test file.

7ee8b05

Fix uni tests and update mocks.

74d6b44

Fix unit test.

c8213a7

Fix lint.

dad3567

Update test file header.

c20adea

Fix lint.

0a6eb31

Only initialize a single index claims manager instance.

5fb09f5

notbdu force-pushed the r/add-claims-for-index-segment-volume-indexes branch from 544f20e to 5fb09f5 Compare November 10, 2020 21:14

notbdu added 3 commits November 10, 2020 17:31

Reject out of retention claims and add unit test for rejection and de…

78b03de

…letion of out of retention claims.

Ensure fs clock options are properly set in integration tests.

b32726b

Adhere to gofumpt linter rules.

2669099

Remove excess log.

f1ededb

notbdu merged commit 251dc3d into master Nov 11, 2020

notbdu deleted the r/add-claims-for-index-segment-volume-indexes branch November 11, 2020 05:14

notbdu restored the r/add-claims-for-index-segment-volume-indexes branch November 12, 2020 05:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dbnode] Add claims for index segments volume index #2846

[dbnode] Add claims for index segments volume index #2846

robskillington commented Nov 6, 2020

notbdu left a comment

notbdu Nov 6, 2020

notbdu Nov 6, 2020

notbdu Nov 6, 2020

notbdu Nov 6, 2020

nbroyles left a comment

notbdu Nov 6, 2020

notbdu Nov 6, 2020

nbroyles Nov 10, 2020 •

edited

notbdu Nov 10, 2020

nbroyles commented Nov 10, 2020

codecov bot commented Nov 10, 2020 •

edited

[dbnode] Add claims for index segments volume index #2846

[dbnode] Add claims for index segments volume index #2846

Conversation

robskillington commented Nov 6, 2020

notbdu left a comment

Choose a reason for hiding this comment

notbdu Nov 6, 2020

Choose a reason for hiding this comment

notbdu Nov 6, 2020

Choose a reason for hiding this comment

notbdu Nov 6, 2020

Choose a reason for hiding this comment

notbdu Nov 6, 2020

Choose a reason for hiding this comment

nbroyles left a comment

Choose a reason for hiding this comment

notbdu Nov 6, 2020

Choose a reason for hiding this comment

notbdu Nov 6, 2020

Choose a reason for hiding this comment

nbroyles Nov 10, 2020 • edited

Choose a reason for hiding this comment

notbdu Nov 10, 2020

Choose a reason for hiding this comment

nbroyles commented Nov 10, 2020

codecov bot commented Nov 10, 2020 • edited

Codecov Report

nbroyles Nov 10, 2020 •

edited

codecov bot commented Nov 10, 2020 •

edited