[dbnode] Avoid loading blocks in memory for namespaces with snapshots disabled during bootstrapping #3919

soundvibe · 2021-11-11T10:46:12Z

Use persist.FileSetFlushType instead of persist.FileSetSnapshotType for second target data and index ranges if bootstrapping namespace has !snapshotEnabled. Because of this change, bootstrappers won't keep their second range blocks in memory for snapshot disabled namespaces (shouldPersist will evaluate to true for them).
This should help to reduce dbnode's memory usage during bootstrapping because snapshot-disabled namespaces usually have long retentions and block sizes so keeping them all in memory could potentially lead to OOM.

What this PR does / why we need it:

Fixes #

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?:

…e` for second target data and index ranges if bootstrapping namespace is read only. Because of this change, bootstrappers won't keep second range blocks in memory for read only namespaces (`shouldPersist` will evaluate to true for them).

soundvibe · 2021-11-11T10:49:02Z

src/dbnode/storage/bootstrap/process.go

 			// If yes, return an error to force a retry
-			if persistConf := ns.DataRunOptions.RunOptions.PersistConfig(); persistConf.Enabled &&
-				persistConf.FileSetType == persist.FileSetSnapshotType {
+			if runIndex == 1 {


I think checking for the second run should be enough here to replace previous condition. This is needed because now second run blocks won't always have persist.FileSetSnapshotType set.

Do I understand correctly that this is not really required to avoid loading blocks in memory for read only? Can we split this into separate PR? Mainly because with my limited knowledge this seem to be a dangerious change that might also affect non read only namespaces and we might need to revert it.

This is needed because if this condition is left unchanged, we won't be able to check if target ranges have advanced for read-only namespaces. Don't think that this change is dangerous, because it will behave in the same way as before (just the condition is different)

Hm, i agree the logic is still similar but i agree - i’d rather not change setting file snapshot type to something other than Snapshot.

I’ll keep reviewing and give recommendation once understanding the larger change, but seems we are going from type safe to type unsafe potentially.

Would this make more sense if we based the decision on "snapshotEnabled": false namespace property (instead of readOnly)?

Agree that decision based on "snapshotEnabled": false could be considered as well.
And for Rob's concerns - I don't think type safety gives us much here. The most important thing we're doing here is checking that target ranges have not advanced for the last run or latest ranges we've calculated before. I am not sure if this should depend on snapshot fileSet type because this is more of the time problem we're solving here and not the type of persistence.

Updated PR to use snapshotEnabled instead.

codecov · 2021-11-11T11:02:41Z

Codecov Report

Merging #3919 (7f2a91f) into master (13e4c45) will decrease coverage by 0.3%.
The diff coverage is 94.2%.

❗ Current head 7f2a91f differs from pull request most recent head e205a44. Consider uploading reports for the commit e205a44 to get more accurate results

@@           Coverage Diff            @@
##           master   #3919     +/-   ##
========================================
- Coverage    56.9%   56.6%   -0.4%     
========================================
  Files         555     555             
  Lines       63456   63326    -130     
========================================
- Hits        36152   35882    -270     
- Misses      24119   24240    +121     
- Partials     3185    3204     +19

Flag	Coverage Δ
aggregator	`62.2% <ø> (+0.1%)`	⬆️
cluster	`∅ <ø> (∅)`
collector	`58.4% <ø> (ø)`
dbnode	`60.3% <94.2%> (-0.5%)`	⬇️
m3em	`46.4% <ø> (ø)`
metrics	`19.7% <ø> (ø)`
msg	`74.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 13e4c45...e205a44. Read the comment docs.

* master: [m3msg] Remove unnecessary ConsumeHandler interface (#3918) [query] Prom converter supporting value decrease tolerance (#3914)

src/dbnode/storage/bootstrap/process.go

Antanukas · 2021-11-11T14:11:13Z

src/dbnode/storage/bootstrap/process.go

 			// If yes, return an error to force a retry
-			if persistConf := ns.DataRunOptions.RunOptions.PersistConfig(); persistConf.Enabled &&
-				persistConf.FileSetType == persist.FileSetSnapshotType {
+			if runIndex == 1 {


Do I understand correctly that this is not really required to avoid loading blocks in memory for read only? Can we split this into separate PR? Mainly because with my limited knowledge this seem to be a dangerious change that might also affect non read only namespaces and we might need to revert it.

robskillington · 2021-11-11T14:44:41Z

src/dbnode/storage/bootstrap/process.go

@@ -215,23 +219,24 @@ func (b bootstrapProcess) Run(
 			},
 		})
 		secondRanges := b.newShardTimeRanges(
-			dataRanges.secondRangeWithPersistFalse.Range, namespace.Shards)
+			dataRanges.secondRange.Range, namespace.Shards)


Hm why are we using “secondRange” instead of “secondRangeWithPersistFalse” here?

Because if namespace is read-only, it will resolve to shouldPersist=true for the second range. Other namespaces will still resolve to shouldPersist=false as it was before these changes.

linasm

LGTM, just asked for some alignments with the recent readOnly -> !snapshotEnabled change (comments inline).

src/dbnode/storage/bootstrap/process_test.go

src/dbnode/storage/bootstrap/process.go

* master: [agg] Use timestamp (not start aligned) for expiring forward versions (#3922) [tests] Add support for calls to label APIs in resources.Coordinator (#3916) [tests] Convert repair_and_replication Docker Integration Test to In-process (#3903) Always Close the conn if failed to write acks (#3855) [m3msg] Add receive and handle latency to consumers (#3920)

soundvibe commented Nov 11, 2021

View reviewed changes

Merge branch 'master' into linasn/bootstrap-readonly-ns

4b27532

* master: [m3msg] Remove unnecessary ConsumeHandler interface (#3918) [query] Prom converter supporting value decrease tolerance (#3914)

soundvibe requested review from vpranckaitis, robskillington and linasm November 11, 2021 14:04

Antanukas reviewed Nov 11, 2021

View reviewed changes

Inline namespace.ReadOnly instead of declaring it as var.

28ceb75

robskillington reviewed Nov 11, 2021

View reviewed changes

soundvibe changed the title ~~[dbnode] Avoid loading blocks in memory for read-only namespaces during bootstrapping~~ [dbnode] Avoid loading blocks in memory for namespaces with snapshots disabled during bootstrapping Nov 15, 2021

soundvibe added 2 commits November 15, 2021 16:01

Use snapshotEnabled instead of namespace.ReadOnly.

c4535ae

Removed unused field.

7f2a91f

soundvibe requested a review from robskillington November 15, 2021 14:46

linasm approved these changes Nov 16, 2021

View reviewed changes

src/dbnode/storage/bootstrap/process_test.go Outdated Show resolved Hide resolved

src/dbnode/storage/bootstrap/process.go Show resolved Hide resolved

soundvibe added 2 commits November 16, 2021 16:02

Updated unit test names to align with snapshotEnabled.

e205a44

soundvibe merged commit e2b4a8a into master Nov 17, 2021

soundvibe deleted the linasn/bootstrap-readonly-ns branch November 17, 2021 09:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dbnode] Avoid loading blocks in memory for namespaces with snapshots disabled during bootstrapping #3919

[dbnode] Avoid loading blocks in memory for namespaces with snapshots disabled during bootstrapping #3919

soundvibe commented Nov 11, 2021 •

edited

Loading

soundvibe Nov 11, 2021

Antanukas Nov 11, 2021 •

edited

Loading

soundvibe Nov 11, 2021

robskillington Nov 11, 2021

linasm Nov 11, 2021

soundvibe Nov 12, 2021

soundvibe Nov 15, 2021

codecov bot commented Nov 11, 2021 •

edited

Loading

Antanukas Nov 11, 2021 •

edited

Loading

robskillington Nov 11, 2021

soundvibe Nov 11, 2021

linasm left a comment

[dbnode] Avoid loading blocks in memory for namespaces with snapshots disabled during bootstrapping #3919

[dbnode] Avoid loading blocks in memory for namespaces with snapshots disabled during bootstrapping #3919

Conversation

soundvibe commented Nov 11, 2021 • edited Loading

Choose a reason for hiding this comment

Antanukas Nov 11, 2021 • 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

codecov bot commented Nov 11, 2021 • edited Loading

Codecov Report

Antanukas Nov 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linasm left a comment

Choose a reason for hiding this comment

soundvibe commented Nov 11, 2021 •

edited

Loading

Antanukas Nov 11, 2021 •

edited

Loading

codecov bot commented Nov 11, 2021 •

edited

Loading

Antanukas Nov 11, 2021 •

edited

Loading